-
Notifications
You must be signed in to change notification settings - Fork 213
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
Bump and unify PDM in the api/ and indexer_worker/ to 2.21 #5250
Conversation
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.
One note on version placement, but otherwise this works locally for me and looks good!
api/Dockerfile
Outdated
&& apt-get autoremove -y \ | ||
&& rm -rf /var/lib/apt/lists/* \ | ||
&& pip install pdm~=2.19 | ||
&& pip install --upgrade pip \ | ||
&& pip install pdm~=2.21 |
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.
We have the Python version defined as API_PY_VERSION
on Line 5, does it maybe make sense to make this package version an similar argument in the same place in the Dockerfile? Even if it just has a default to the pinned version.
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.
That makes sense, it would also make it easier to bump PDM everywhere by changing it in one place!
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 tried to do that and pass the version from the just file (then compose.yml -> Dockerfile), but it wasn't taking the value from the env var. As you suggest, I can add it here anyway with the default value. It would make it easy to find the values to replace later!
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.
Approving this, but with a +1 to @AetherUnbound's idea. Up to you if you want to pursue that as a separate PR.
Folks, I managed to centralize the version in an env var from the justfile. It was all about the placement since, in the Dockerfile, each |
Latest k6 run output1
Footnotes
|
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 is great!
We will need to manually remember to update PDM from time to time because Renovate would not look for the version number in justfile
.
@dhruvkb Correct, the same situation as currently because renovate wasn't updating PDM anyway. |
True, that was just a mental note and not even remotely a critique of this PR. |
Ahhh, that's excellent! I'm glad it was possible to centralize that! |
Description
Follows the series of dependency updates covering Renovate missings.
Testing Instructions
Confirm the API continues to run normally. Watch the CI workflow passing.
Checklist
Update index.md
).main
) or a parent feature branch.ov just catalog/generate-docs
for catalogPRs) or the media properties generator (
ov just catalog/generate-docs media-props
for the catalog or
ov just api/generate-docs
for the API) where applicable.Developer Certificate of Origin
Developer Certificate of Origin