-
Notifications
You must be signed in to change notification settings - Fork 356
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
ci: no det version parameter via continued config #10151
base: main
Are you sure you want to change the base?
ci: no det version parameter via continued config #10151
Conversation
✅ Deploy Preview for determined-ui canceled.
|
9e93049
to
a94ed82
Compare
c54a022
to
d2851e2
Compare
9890b21
to
1c291e6
Compare
e342bb3
to
65d14ab
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10151 +/- ##
=======================================
Coverage 54.26% 54.26%
=======================================
Files 1259 1259
Lines 157276 157276
Branches 3642 3641 -1
=======================================
+ Hits 85344 85345 +1
+ Misses 71799 71798 -1
Partials 133 133
Flags with carried forward coverage won't be shown. Click here to find out more. |
dd80918
to
ac82fc2
Compare
ac82fc2
to
70a34cd
Compare
b0b3776
to
333ff1e
Compare
2c9d075
to
8a7c7d8
Compare
0ad48ea
to
49f01f4
Compare
09485c9
to
e45d631
Compare
86b758a
to
5f66e3a
Compare
5f66e3a
to
63ffe35
Compare
@@ -13,15 +13,12 @@ orbs: | |||
executors: | |||
python-38: | |||
docker: | |||
- image: python:3.8-slim-bookworm |
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.
these didnt have git installed, so they couldn't run the script
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.
Good catch!
package-location: | ||
type: string | ||
steps: | ||
- run: | ||
name: Install <<parameters.package-name>> |
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.
this previously required pipeline.parameters.det-version
, so idk why were were parameterizing on package name
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.
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.
It looks like this could install any package the same way instead of only installing Determined. 🤷♂️ If it's only used for one package, then I agree that the parameter is unecessary.
(FWIW, the blame probably shows that I wrote all this stuff because I renamed the config file when switching to dynamic config. :))
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.
ohh okay! thanks for looking
Ticket
https://hpe-aiatscale.atlassian.net/browse/INFENG-944
Description
The det-version parameter wasn't updating on rerun. Since we use a script, let's just use the script at runtime instead of config-time.
Test Plan
Checklist
docs/release-notes/
See Release Note for details.