-
Notifications
You must be signed in to change notification settings - Fork 50
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.
Isn't *
supposed to mean the latest version? Why is it using 1.6.0 when 1.8.0 is available?🤔
Pressed approve accidentally...
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's a problem with hashes in CI:
#15 86.98 [pipenv.exceptions.InstallError]: Using cached sentry_sdk-1.8.0-py2.py3-none-any.whl (153 kB)
#15 86.98 [pipenv.exceptions.InstallError]: ERROR: THESE PACKAGES DO NOT MATCH THE HASHES FROM THE REQUIREMENTS FILE. If you have updated the package versions, please update the hashes. Otherwise, examine the package contents carefully; someone may have tampered with them.
#15 86.98 [pipenv.exceptions.InstallError]: sentry-sdk==1.8.0 from https://files.pythonhosted.org/packages/32/63/f3e04e5d34fc110aa1ff323c8936c922a4a2d9ac988391b60410c3fc17f2/sentry_sdk-1.8.0-py2.py3-none-any.whl#sha256=5daae00f91dd72d9bb1a65307221fe291417a7b9c30527de3a6f0d9be4ddf08d (from -r /tmp/pipenv-dg6ymxx4-requirements/pipenv-uypevpgz-requirement.txt (line 1)):
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.
To fix the error Olga found, I think we should follow the steps in this comment: pypa/pipenv#2665 (comment)
API Developer Docs Preview: Ready https://wordpress.github.io/openverse-api/_preview/825 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
I had precisely the same doubt... 🤷🏻♀️ And now the v1.9.0 is out. Tried to avoid updating more packages but seems impossible if we need to lock the Pipfile. Anyway, the rest updates are at most minor changes, so It's not such a drastic change at least. |
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!
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 seems that all dependencies got updated, not just sentry-sdk
. This is how Pipenv does things by default and it takes extra effort to prevent it from doing so.
Do we care? Do we want to be more precise? We could copy the Pipfile updates for sentry-sdk entry only, but we'd miss transient dependencies that updated as well in that case 😞
Requesting changes just to make sure we're confident in the changes we're making in this PR. If we are, feel free to dismiss and request a re-review with details of the plan here. |
Doesn't seem to be any way to prevent it, when I ran
I believe that will still leave the Pipenv.lock file in an "out of date" or inconsistent state, maybe? Because it uses a hash to verify the dependencies are aligned I believe. |
I'm not sure what selective upgrade is meant to do 🤔 This comment makes me think it is possible to upgrade just a single package following the manual copying flow: pypa/pipenv#2665 (comment) It's pretty annoying though... but updating all at once is arguably risky? Do other folks feel okay with upgrading all dependencies in one go? |
Following instructions from pypa/pipenv#2665 (comment) pypa/pipenv#2665 (comment)
It is very annoying indeed and seems risky since it needs to be repeated manually for dependencies of the updated package, but given that
It's a bit risky but is what we have been doing with dependabot updates all this time anyway 🤷🏻♀️ We'll see it again on Monday. We have been relying mostly on tests to catch any possible breaking change until now. |
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.
Looks good to me this way.
FWIW, I'm not sure how Dependabot does it but it does only update the specific dependencies and not any others. You can see that in this PR: https://github.com/WordPress/openverse-api/pull/794/files
Only boto3
and its dependencies have hashes updated.
Is it You can see this other dependabot PR #739 (files) where it says it updates only |
Wow, so it is. That is really wild. Am I missing something? I don't understand why a dependency management tool would work this way. |
Description
It's curious that dependabot hasn't updated this before. I followed instructions in the pypa/pipenv#2665 (comment) suggested by @sarayourfriend.
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin