feat: add Python 3.14 to samples testing image#13596
Conversation
417c600 to
3a2ae38
Compare
There was a problem hiding this comment.
Code Review
This pull request updates the Python versions in the testing Docker image, adding support for Python 3.14 and refreshing patch versions for others. My feedback focuses on improving the readability and maintainability of the Dockerfile where these versions are listed.
| 3.12.11 \ | ||
| 3.13.8 \ | ||
| 3.14.0" \ | ||
| && for PYTHON_VERSION in $PYTHON_VERSIONS; do \ |
There was a problem hiding this comment.
I'd prefer to avoid variables that differ only by one letter. How about
for VERSION in $PYTHON_VERSIONS or for PYTHON_VERSION in $ALL_PYTHON_VERSIONS?
| && echo "disable-ipv6" >> "${GNUPGHOME}/dirmngr.conf" \ | ||
| && /tmp/fetch_gpg_keys.sh \ | ||
| && PYTHON_VERSIONS="\ | ||
| 2.7.18 \ |
There was a problem hiding this comment.
This commit removes Python 2.7 entirely from the base image. Will this affect CI?
Per an earlier update, #13066 (comment) indicates this was going to be removed, but it's still an active check on this PR. I also recall an earlier PR (can't find it now) where removing Python 2.7 breaks kokoro. Was that resolved? I'm cautious of an update that adds a new python version but also coincidently removes an older major version.
|
Guessing based on #12780 that we can't test the new image works until after this PR is merged and new changes come in? I'm also guessing that the changes to this Docker image aren't tested until after this PR is merged and the image is rebuilt? (Automatically via a Cloud Build trigger, by the look of it). Is there a manual way to test that the Dockerfile changes work before merging the PR? (The failing tests here appear to all be related to individual tests not working, which I will begin addressing separetely.) |
f3c7054 to
40f6646
Compare
iennae
left a comment
There was a problem hiding this comment.
There are some samples that need to be updated; we're prioritizing new infra and unblocking samples that need the new runtimes.

Similar to #12780