Fix test script to use launcher.enabled and strict bash flags#792
Fix test script to use launcher.enabled and strict bash flags#792
Conversation
- Use `launcher.enabled=false` instead of `config.Launcher.Enabled=false` - Add `-euxo pipefail` for stricter bash error handling
| - /opt/python/3.12.11/bin/python | ||
| - /opt/python/3.11.13/bin/python | ||
| - /opt/python/3.13.9/bin/python |
There was a problem hiding this comment.
Is this a significant change for end-users? Is there any reason to keep 3.11 around?
There was a problem hiding this comment.
It is. Unfortunately this config change was made upstream in the docker image's default configuration so it's too late to go back now.
cc @ianpittwood
There was a problem hiding this comment.
Oh, we only support 2 versions of Python? 😞
There was a problem hiding this comment.
@dbkegley Connect was significantly behind the other products in its dependency versions to where I had some issues raised related to versions being out of sync. I thought I had gotten approval to make these changes, but it wasn't given enough thought as is clear by the fact I forgot to update the charts.
We should have a conversation about how we can better manage synchronizing the image config with the helm config going forward and/or accelerate our move to the new images repos. Since Helm overrides the configs in the image, which we do try to keep up to date, it's always going to have a lagging problem unless we modify our release workflow. The new images should make that process easier since it will heavily diminish our chances of making a breaking in-release change due to each release pinning to versions present when the release is initially created. We should at least add a step to check dependency versions and consider upgrades/patches when updating the image and the Helm charts, with particular scrutiny over making sure the Helm chart dependency versions will match the image dependency versions for the tag being targeted.
As @kfeinauer said, we can provide multiple Python versions in the Connect image, but it's an image size issue for rstudio-docker-products. The new images use uv which significantly cuts down the Python installation size, but this repo still uses python-builds. We can include an additional version to mitigate the issue if you want, but it will inflate the image by around 700mb if memory serves. It's not as hefty as additional R versions at least.
There was a problem hiding this comment.
@ianpittwood Can weigh in more here, but we can support more than 2 versions of Python, but with the current way of distributing Python in the images it is quite expensive in terms of size (the new images will use uv which is much more space efficient).
We could add back in Python 3.11.13 alongside the newer 3.13.9 to prevent a breakage for customers using Helm. Should we consider going down this route (and taking the size penalty), or should we proceed with a version break?
We should have consulted you more about upgrading Python, so apologies there. Connect was very behind the other products on Python version so we decided to update to keep the products more in parity, but we understand that dropping Python versions is a big deal for Connect so we should have gotten buy-in before doing so.
There was a problem hiding this comment.
Totally understand why the change was made since we were way behind. I just wanted to make sure it was flagged so we could start the discussion about how we can change the process going forward. Right now it's challenging for us to time these upgrades in order to avoid breakages. We like to coordinate the upgrades with Connect version bumps which allows us to call out the change in the image/helm release notes and give customers a chance to plan to the change proactively.
We should at least add a step to check dependency versions and consider upgrades/patches when updating the image and the Helm charts, with particular scrutiny over making sure the Helm chart dependency versions will match the image dependency versions for the tag being targeted.
This PR fixes the CI checks that enforce this for the helm chart but we are lacking similar checks on the images repo upstream.
We could add back in Python 3.11.13 alongside the newer 3.13.9 to prevent a breakage for customers using Helm. Should we consider going down this route (and taking the size penalty), or should we proceed with a version break?
Given that this has been broken for a while now, I don't think we need to worry about re-adding the old version.
We should have a conversation about how we can better manage synchronizing the image config with the helm config going forward and/or accelerate our move to the new images repos. Since Helm overrides the configs in the image, which we do try to keep up to date, it's always going to have a lagging problem unless we modify our release workflow. The new images should make that process easier since it will heavily diminish our chances of making a breaking in-release change due to each release pinning to versions present when the release is initially created.
Agreed, let's move this discussion offline for now and make an announcement once we've reached a consensus on how we want to move forward. I think the migration to the new images is one part of the equation. I'd like to explore what it would look like to make the min images the helm chart defaults and rely more on executable scanning so we can eliminate the need to explicitly configure the interpreter versions.
Fix connect-version-check in CI and update documentation and NEWS
Allow empty executable lists with strict bash flags
5 participants
launcher.enabled=falseinstead ofconfig.Launcher.Enabled=false-euxo pipefailfor stricter bash error handlingRelated to #791 - CI is supposed to catch this case but it's been failing silently for a while.
and also: rstudio/rstudio-docker-products#1008