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

feat: Added check to verify pip.txt installation #271

Merged
merged 1 commit into from
May 25, 2022

Conversation

Jawayria
Copy link
Contributor

@Jawayria Jawayria commented May 20, 2022

This check verifies that we are installing pip immediately after upgrading it.
pip should be installed after the upgrade to ensure that new pip version is compatible with installed pip-tools version.

JIRA: https://2u-internal.atlassian.net/browse/BOM-3402

@codecov
Copy link

codecov bot commented May 20, 2022

Codecov Report

Merging #271 (8530940) into master (5f976de) will increase coverage by 0.21%.
The diff coverage is 100.00%.

❗ Current head 8530940 differs from pull request most recent head 580a661. Consider uploading reports for the commit 580a661 to get more accurate results

@@            Coverage Diff             @@
##           master     #271      +/-   ##
==========================================
+ Coverage   82.47%   82.68%   +0.21%     
==========================================
  Files          21       21              
  Lines        1124     1138      +14     
==========================================
+ Hits          927      941      +14     
  Misses        197      197              
Flag Coverage Δ
unittests 82.68% <100.00%> (+0.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
repo_health/__init__.py 64.28% <100.00%> (ø)
repo_health/check_makefile.py 93.93% <100.00%> (+4.46%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f976de...580a661. Read the comment docs.

@Jawayria Jawayria requested a review from jmbowman May 23, 2022 11:09
Copy link
Contributor

@jmbowman jmbowman left a comment

Choose a reason for hiding this comment

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

This looks like a good start; can we also check that the updated pip-tools.txt is also reinstalled? I'd really also like a check that pip.txt is correctly utilized in any code that initializes an environment (like make requirements and tox.ini), but we probably need to manually make sure we can do that correctly in a few places before establishing a check for it.

@@ -18,6 +18,8 @@ piptools:

export CUSTOM_COMPILE_COMMAND = make upgrade
upgrade: piptools ## update the requirements/*.txt files with the latest packages satisfying requirements/*.in
pip-compile --allow-unsafe --rebuild --upgrade -o requirements/pip.txt requirements/pip.in
pip install -qr requirements/pip.txt
pip-compile --rebuild --upgrade -o requirements/pip_tools.txt requirements/pip_tools.in
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to use the old, known-working pip and pip-tools conversation to update both pip.txt and pip-tools.txt, then reinstall packages from both updated files to try the new combination. So the pip.txt installation should be after the pip-tools.txt recompilation. The check above should also be updated accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jmbowman I've updated the check, can you please have a look?

Copy link
Contributor

@jmbowman jmbowman left a comment

Choose a reason for hiding this comment

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

Looks good! We probably have a lot of repos that will need work to make this pass, even the edx-cookiecutters repo gets it wrong.

@Jawayria
Copy link
Contributor Author

Looks good! We probably have a lot of repos that will need work to make this pass, even the edx-cookiecutters repo gets it wrong.

Yes you're right

@Jawayria Jawayria force-pushed the jawayria/check-upgrade-target branch from 8530940 to 580a661 Compare May 25, 2022 11:01
@Jawayria Jawayria merged commit 7e2738d into master May 25, 2022
@Jawayria Jawayria deleted the jawayria/check-upgrade-target branch May 25, 2022 11:02
This was referenced Jun 1, 2022
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.

3 participants