-
-
Notifications
You must be signed in to change notification settings - Fork 33
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
Upgrade pip to 20.3.3 and virtualenv to 20.4.0 #98
Conversation
You can check the https://hub.docker.com/repository/registry-1.docker.io/stackstorm/buildpack/builds/f12894d9-f84b-4842-a2c5-7fea9e8abd7e build at Docker Hub. Copy-paste error log:
|
Putting back to draft whilst we fix why still getting complaints about no-site-packages. Also may want to use pip 20.3.3 as it matches the version used by virtualenv. |
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.
packagingbuild/bionic/Dockerfile
Outdated
RUN apt-get -y install \ | ||
python-virtualenv python-setuptools python-mock && \ | ||
apt-get clean && \ | ||
git clone --branch stackstorm_patched_use_custom_pip https://github.com/StackStorm/dh-virtualenv.git /tmp/dh-virtualenv && \ | ||
git clone --branch rm_site_packages_upgrade_pip https://github.com/StackStorm/dh-virtualenv.git /tmp/dh-virtualenv && \ |
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.
Per my comment on Slack - please open PR with your changes against stackstorm_patched
branch in dh-virtualenv repo and let's just use stackstorm_patched
branch.
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 assume you will update this line here when the other PR is merged, right?
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.
@Kami Yes - updated now - to use stackstorm_patched.
packagingbuild/xenial/Dockerfile
Outdated
# This line just busts Docker's cache so it re-runs the next line | ||
# The GitHub API will return different results when the branch HEAD changes | ||
# See https://stackoverflow.com/a/39278224 | ||
# ADD https://api.github.com/repos/StackStorm/dh-virtualenv/git/refs/heads/rm_site_packages_upgrade_pip version.json |
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 option would also to be pin to check out a specific commit revision in the git clone / checkout line
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 approved this PR, but let's please also make the dh-virtualenv change mentioned in the comment.
You got rid of the buildpack empty files. 👍 |
Oops - missed them. Thanks - hopefully got them all now! |
TODO: Update the PR to use stackstorm-patched branch after the dh-virtualenv PR has merged changes into stackstorm-patched branch. Will push up that change after other PR merged.... |
@armab Would you be able to re-review, as I believe your review comments have now been addressed. Thanks. |
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 👍
# | ||
# We use our dh-virtualenv version, since it fixes shebangd lines rewrites. | ||
# The shebangd rewrites are in master branch, but that requires dependency | ||
# on debhelper 12. |
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 think in the future it's OK to switch to debhelper 12
.
From what I see we're on 10
right now: https://github.com/StackStorm/st2-packages/blob/master/packages/st2/debian/compat
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.
When I tried on bionic in our images, the latest debhelper it could download was 11. I don't think 12 is in the bionic repos.
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.
Makes sense, good to know!
So probably 10
for Ubuntu Xenial then.
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 👍
Thanks, everyone!
Using pip 20.3.3 and legacy resolver