feat: Migrate shared test/quality related workflows from kubeflow/kubeflow to notebooks-v1 branch #612
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| name: Common Frontend Tests | ||
| on: | ||
| pull_request: | ||
| paths: | ||
| - components/crud-web-apps/common/frontend/kubeflow-common-lib/** | ||
| - releasing/version/VERSION | ||
| branches: | ||
| - main | ||
| - v*-branch | ||
| - notebooks-v1 | ||
|
Comment on lines
+9
to
+10
There was a problem hiding this comment. Just for consistency with other workflows - can you include
There was a problem hiding this comment. Hey Andy, thank you for the initial review. There was a problem hiding this comment. @thesuperzapper had expressed to leave this in for now... so I am honoring that preference... eventually something will move to There was a problem hiding this comment. Thank you for the information. I will make sure to add it. |
||
|
|
||
| env: | ||
| FRONTEND_DIR: components/crud-web-apps/common/frontend/kubeflow-common-lib | ||
|
|
||
| jobs: | ||
| frontend-format-lint-check: | ||
| name: Check code format and lint | ||
| runs-on: ubuntu-22.04 | ||
| defaults: | ||
| run: | ||
| working-directory: ${{ env.FRONTEND_DIR }} | ||
|
|
||
| steps: | ||
| - name: Check out code | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup Node | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 16 | ||
|
|
||
| - name: Install dependencies | ||
| run: npm ci | ||
|
|
||
| - name: Check frontend code formatting | ||
| run: npm run format:check | ||
|
|
||
| - name: Check frontend code linting | ||
| run: npm run lint-check | ||
|
|
||
| frontend-unit-tests: | ||
| runs-on: ubuntu-22.04 | ||
| name: Unit tests | ||
| defaults: | ||
| run: | ||
| working-directory: ${{ env.FRONTEND_DIR }} | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup node version to 16 | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 16 | ||
|
|
||
| - name: Install Kubeflow common library dependencies | ||
| run: npm ci | ||
|
|
||
| - name: Run unit tests | ||
| run: npm run test:prod | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| name: Python Linting | ||
|
|
||
| on: | ||
| pull_request: | ||
| branches: | ||
| - main | ||
| - v*-branch | ||
| - notebooks-v1 | ||
| paths: | ||
| - "**.py" | ||
|
|
||
| jobs: | ||
| flake8-lint: | ||
| runs-on: ubuntu-22.04 | ||
| name: Check | ||
| steps: | ||
| - name: Checkout source repository | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up Python environment 3.8 | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: "3.8" | ||
|
|
||
| - name: flake8 Lint | ||
| uses: py-actions/flake8@v2 | ||
| with: | ||
| exclude: "docs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I have confirmed this PR does successfully migrate the
common_frontend_tests.yamlappropriately - I take issue with the original state of the workflow 😈Please consider this example commit I made while verifying your contribution:
There are a few things going on here that I feel might warrant inclusion:
env.FRONTEND_DIRto avoid having to hardcode this path in multiple placesdefaults.run.working-directoryto avoid having to explicitlycdinto the directory in eachrun:blocknpm ci(instead ofnpm i) for a more deterministic buildfrontend-format-lint-checkAnother change included that I feel less sure about is the
cachechanges toSetup Nodesteps in each job..kubeflow/notebookshas codebases acrossnotebooks-v1andnotebooks-v2branches - 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 changeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.