feat: Migrate shared test/quality related workflows from kubeflow/kubeflow to notebooks-v1 branch

[liavweiss]

feat: migrate shared test/quality related workflows from kubeflow/kubeflow to notebooks-v1 branch

Enables python_lint.yaml and common_frontend_tests.yaml tests to run in kubeflow/notebooks repository.

I tested these tests with a small modification in the relevant paths (I have already reverted these changes).

Solves issue: #593

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kimwnasptd for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[andyatmiami]

/ok-to-test

[andyatmiami]

@liavweiss

Thanks for opening this up...

I have a few requests here for optimizations here I am happy to discuss with you... but none of them are particularly "critical"

Bare minimum change would be to simply include main as a branch target for sake of consistency.

[andyatmiami]

Just for consistency with other workflows - can you include main in the branches: argument

• I wholly realize/appreciate this is essentially a no-op!

[liavweiss]

Hey Andy, thank you for the initial review.
I agree that we need to keep the consistency here, but maybe we need to consider removing the main branch from the other PRs? Do we have a plan to use this branch? It looks like after the split of notebooks-v1 and notebooks-v2 branches, the main branch can be "deleted"

[andyatmiami]

@thesuperzapper had expressed to leave this in for now... so I am honoring that preference...

eventually something will move to main branch... so this gives us freedom to easily in the future decide to move notebooks-v1 or notebooks-v2 and not require changes to workflow if main already included.

[liavweiss]

Thank you for the information. I will make sure to add it.

[andyatmiami]

While I have confirmed this PR does successfully migrate the common_frontend_tests.yaml appropriately - I take issue with the original state of the workflow 😈

Please consider this example commit I made while verifying your contribution:

• andyatmiami@5be069f

There are a few things going on here that I feel might warrant inclusion:

• env.FRONTEND_DIR to avoid having to hardcode this path in multiple places
• defaults.run.working-directory to avoid having to explicitly cd into the directory in each run: block
• npm ci (instead of npm i) for a more deterministic build
  • also only run it once in frontend-format-lint-check

Another change included that I feel less sure about is the cache changes to Setup Node steps in each job..

• The code does appear to work - but I wasn't seeing that much of a speed increase
• Given kubeflow/notebooks has codebases across notebooks-v1 and notebooks-v2 branches - each with multiple components - as well as multi-arch support... I worry the 10GB cache limit per repo in GitHub would result in a lot of cache misses - kinda undermining this change
  • logging output claimed the cache "just" for common lib was 96MB.. which seems insanely large - but hey, Angular gonna Angular 🤷‍♂️

[liavweiss]

I agree with you on those changes, thank you for raising them.

Regarding the cache, I ran a quick comparison on the common lib workflow with and without the npm cache. The results showed only about a ~9s improvement on the format/lint job, while the unit test job actually ran ~4s slower (maybe due to cache overhead).
Given that, plus your point about multiple branches/components and the 10GB repo-wide cache limit, I agree that caching here likely won’t give us consistent wins and could end up causing frequent cache misses.

[liavweiss]

@andyatmiami, pr is ready for a final review.