Skip to content
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

Handle Brotlipy outdated hashes, option 2 (of many) - pipenv selective-upgrade #76

Merged
merged 1 commit into from
Dec 1, 2022

Conversation

cmharlow
Copy link
Contributor

@cmharlow cmharlow commented Dec 1, 2022

Goal

Currently, #74 is blocked by the CircleCI container environment wanting to install a copy of brotlipy-0.7.0 Python package where its SHA from upstream doesn't match any of the SHAs in the Pipfile.lock.

One way to approach repairing this is explicitly installing the currently used version of brotlipy, which then adds & updates some brotlipy dependencies. This is the command used:

pipenv install --selective-upgrade --keep-outdated brotlipy==0.7.0

If we really want to restrict updates, we can pin the dependencies that are updated here (importlib-metadata, zipp). I'm considering new dependencies added as okay, but open to discussion on that. I've confirmed the hashes added for brotlipy include all the previously-used hashes as well (you can verify by checking out the Pipfile.lock diff).

Todos:

  • If we decide to go this route with Pipfile updates, probably should just add this new commit to the relud-patch-1 branch then proceed with the original PR (Handle shim data from GLEAN #74) - or, rebase this branch to remove the relud-patch-1 commits and merge this PR independently of Handle shim data from GLEAN #74.
  • ran CircleCI tests_unit job locally to confirm this repairs the issue;

Implementation Decisions

This goes the approach of using Pipenv in as restricted a way as is feasible with the tool to get the latest hashes from pypi for brotlipy version 0.7.0. Unfortunately, it seems like that involves explicitly installing brotlipy, pinned to the version used previously (determined via Pipfile.lock on main branch), and just accepting the dependencies (new & updated).

An alternative implementation to this is via this PR #77 .

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

nina-py
nina-py previously approved these changes Dec 1, 2022
Copy link
Contributor

@nina-py nina-py left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@cmharlow cmharlow merged commit 1a016f1 into main Dec 1, 2022
@cmharlow cmharlow deleted the brotlipy-option-2 branch December 1, 2022 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants