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

Specify Python at minor version instead of patch #1631

Merged
merged 2 commits into from Mar 19, 2018

Conversation

Projects
None yet
2 participants
@jseppi
Contributor

jseppi commented Mar 19, 2018

Closes #1630. See issue for details.

@jseppi jseppi self-assigned this Mar 19, 2018

@jseppi jseppi requested a review from toolness Mar 19, 2018

Dockerfile Outdated
@@ -1,4 +1,4 @@
FROM python:3.6.4

This comment has been minimized.

@toolness

toolness Mar 19, 2018

Contributor

Hmm, see the comment I made here:

To be honest I noticed there are some pretty significant changes between the first version of 3.6 and 3.6.2, so I wouldn't mind us making it explicit in Docker-related stuff, lest we use new features introduced in a patch release and then someone's dev environment breaks because they're on the older version of Python but there's no way for docker to know the minor version changed.

In other words, say 3.6.9 comes out and adds a very minor new feature that we take advantage of. As soon as we push this to cloud.gov, cloud.gov will be fine because it knows a new version of Python has been released and will use the latest version.

HOWEVER, when a dev runs ./docker-update.sh (or docker-compose build), the new version of python will not be pulled from Docker Hub because docker will see the older version of 3.6 in its cache and just use that. While there might be some fancy way to tell Docker to always consult Docker Hub to see if a new patch release has come out, we should also realize this can slow things down, especially if a dev happens to be tethering... eh.

This comment has been minimized.

@jseppi

jseppi Mar 19, 2018

Contributor

Does Python really add features in patch version updates? Looks all like patches/bugfixes in https://docs.python.org/3/whatsnew/changelog.html

This comment has been minimized.

@toolness

toolness Mar 19, 2018

Contributor

Yeah, this is where the line between "feature" and "bugfix" becomes really blurry: in my case, I was working on a personal project a few months ago and one of my machines had 3.6.0 while the other had 3.6.2. I wrote my code on the one with 3.6.2, and unknowingly took advantage of its zipfile does not support pathlib fix. Then when I ran my code on the other machine, I got an exception, which actually took a while to debug--I kept assuming the problem was occurring because of other environmental differences in the two computers' setups, and it wasn't after a while that I tried upgrading my Python version.

I saw this as a new feature, but I guess it could be interpreted as a bugfix because 3.6 was supposed to support the use of Path objects instead of raw strings across the whole standard library, but apparently they forgot about the zipfile module. (This kind of thing is, I think, the basis of the arguments of some vociferous critics of semver.)

Anyways, I realize that updating the minor version number is kind of annoying but IMO it is still less annoying than running into the bug I ran into, because the one I ran into took more time to diagnose and fix 😫

Dockerfile Outdated
@@ -1,4 +1,4 @@
FROM python:3.6
FROM python:3.6.4

This comment has been minimized.

@jseppi

jseppi Mar 19, 2018

Contributor

Changed this back to 3.6.4, @toolness, and added a comment in the associated test.

@toolness

Cool! Um, I can only think of two edge cases now:

  1. A new minor version of Python is released, which cloud.gov supports, but we don't upgrade our Docker setup to use. Technically it's possible (though very unlikely) that our code works on local setups because of a bug in the older version of Python; if a bugfix in the newer version of Python that cloud.gov uses actually breaks our app, this could result in an unfortunate dev/prod mismatch.

  2. A new patch version of Python is released today and we immediately update our Docker setup, and perhaps make a change that takes advantage of some bugfix/feature in the new version, but cloud.gov (or something upstream of them) doesn't yet have a buildpack available for this version. However, because we don't explicitly specify the patch version in our runtime.txt, cloud.gov deploys using the old version of Python without complaint, and we have a dev/prod mismatch.

Personally I think both of these edge cases are very unlikely, but if they do occur, they might be annoyingly hard to diagnose and debug. IMO it's worth the annoyance of manually updating the patch version to prevent situations like this, but since you're usually the one issuing the PRs to change the Python version, I think you're a more important stakeholder than me, so I'll leave the decision to you!

@jseppi

This comment has been minimized.

Contributor

jseppi commented Mar 19, 2018

Great, thanks for the enumeration of potential issues. I think the likelihood of those cases happening are sufficiently low to proceed.

One kind of big issue that this PR does fix is when the Cloud Foundry Python buildpack drops support for older Python versions, our deploys break (as I noticed today with the 3.6.2 version we were on). Given that the patch releases can and typically do contain security fixes, I think making this process more streamlined (ie, just re-running the last good build+deploy from Circle) makes this change worth it.

@jseppi jseppi merged commit 9b273c1 into develop Mar 19, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codeclimate All good!
Details
codeclimate/total-coverage 96% (0.0% change)
Details

@jseppi jseppi deleted the py-minor-version branch Mar 19, 2018

@toolness

This comment has been minimized.

Contributor

toolness commented Mar 19, 2018

Ah good point about the breakage! Noice, sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment