-
Notifications
You must be signed in to change notification settings - Fork 68
feat: Migrate TWA test-related workflows #627
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Migrate TWA test-related workflows #627
Conversation
01e48e4 to
a5bfa91
Compare
8b9b78f to
28f1111
Compare
andyatmiami
left a comment
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.
Really nice investigations/implementations here @yehudit1987 - but for the immediate needs of notebooks-v1 migration - I would prefer we dial back some of these changes to align more with the "status quo" of how kubeflow/kubeflow implemented these workflows.
2615d17 to
a97625b
Compare
a97625b to
09f01c5
Compare
andyatmiami
left a comment
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.
Hey @yehudit1987 - thank you for these changes...
Things look pretty good - but would like for yourself and/or @kimwnasptd to weigh in on the observations I have included on this PR for discussion.
| "@typescript-eslint/eslint-plugin": "4.28.2", | ||
| "@typescript-eslint/parser": "4.28.2", | ||
| "cypress": "~13.13.1", | ||
| "cypress": "14.1.0", |
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.
I have confirmed this 14.1.0 version does fix the connectivity issues b/w Cypress and Firefox... but unfortunately leads to this message cropping up in build logs:
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE package: '[email protected]',
npm WARN EBADENGINE required: { node: '^18.0.0 || ^20.0.0 || >=22.0.0' },
npm WARN EBADENGINE current: { node: 'v16.20.2', npm: '8.19.4' }
npm WARN EBADENGINE }
By all accounts this WARN is not affecting anything - but wanted to surface this so we can discuss what to do.
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.
I am also unclear if we want to fix the version of Cypress to 14.1.0 - or use a qualifier like ^ or ~
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.
Regarding the warning, upgrading the Node version to 18 or 20 would resolve it. this mainly impacts CI/CD environments, so there’s likely no real harm. Alternatively, we could just ignore the warning since it doesn’t affect runtime behavior.
As for the version qualifier, I’m not an expert here, but I’d lean toward using ~ since it allows patch updates while avoiding unexpected minor version changes, a safer middle ground than using ^.
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.
I'd personally be OK with fixing the version to 14.1.0. And to cross-reference our discussion in the other PR as well #625 (comment)
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.
Hey @kimwnasptd and @andyatmiami.
I’ve tested the suggested changes from the other PR. I kept the Cypress version pinned as it was and also pinned the tough-cookie version as recommended.
For this PR’s requirements, it wasn’t strictly necessary, but as expected, it didn’t break anything and helps keep things aligned with the other PR and consistent across the project.
Was tested in the attached - https://github.com/yehudit1987/notebooks/actions/runs/18817889000
| make docker-build | ||
| - name: Install KinD | ||
| run: ./components/testing/gh-actions/install_kind.sh |
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 not directly related to changes introduced in this PR - given the AWS outage today - I am seeing issues trying to retrieve the kind binary referenced in install_kind.sh
➜ Downloads/ $ curl -vL -o kind "https://kind.sigs.k8s.io/dl/v0.22.0/kind-linux-amd64"
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0* Host kind.sigs.k8s.io:443 was resolved.
* IPv6: 2600:1f18:16e:df01::259, 2600:1f18:16e:df01::258
* IPv4: 18.208.88.157, 98.84.224.111
* Trying 18.208.88.157:443...
* Trying [2600:1f18:16e:df01::259]:443...
* Immediate connect fail for 2600:1f18:16e:df01::259: No route to host
* Trying [2600:1f18:16e:df01::258]:443...
* Immediate connect fail for 2600:1f18:16e:df01::258: No route to host
0 0 0 0 0 0 0 0 --:--:-- 0:01:09 --:--:-- 0
It seems GitHub also hosts this binary and it available here:
https://github.com/kubernetes-sigs/kind/releases/download/v0.22.0/kind-linux-amd64
And I have confirmed its presently accessible:
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0
100 6245k 100 6245k 0 0 17.4M 0 --:--:-- --:--:-- --:--:-- 17.4M
Should install_kind.sh be updated to use this alternative location?
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.
I guess this wondering depends on the answer to the question which of the two is more reliable in general?
|
/ok-to-test |
09f01c5 to
979501e
Compare
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.
/lgtm
Thanks for all this work @yehudit1987 - appreciate your responsiveness and diligence in working through issues in this PR (and taking lessons learned to help others facing similar issues 💯 )
I'm comfortable with the changes and have verified the workflows behave as expected.
Tested on my fork:
|
Thank you so much for this effort @yehudit1987! /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kimwnasptd The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
There seems to be a conflict with the |
Signed-off-by: Yehudit Kerido <[email protected]>
979501e to
bf807b4
Compare
|
/lgtm latest commit simply is a result of a rebase to resolve a merge conflict in the |
Migrate frontend tests, multi-arch build, and integration test workflows
Resolve issue [TASK] Migrate TWA test-related workflows from kubeflow/kubeflow to notebooks-v1 branch #589