connect: update content images and enable quarto by default

rstudio / helm

Helm Resources for RStudio Products

MIT License

34 stars 28 forks source link

connect: update content images and enable quarto by default #486

Closed tnederlof closed 5 months ago

tnederlof commented 5 months ago

Adds in the new content-base and content-pro images in the default-runtime.yaml and default-runtime-pro.yaml files created via this PR: https://github.com/rstudio/rstudio-docker-products/pull/737
Enables Quarto and Python in the default values.yaml config section so Python and Quarto content will work out of the box for local execution or off-host execution (closes https://github.com/rstudio/helm/issues/463). Previously Python was automatically enabled only when launcher was enabled. This conditional is removed now that its in the values.

Testing

Deployed Connect into EKS using base and pro, they show up as expected and content can be deployed from R and Python (tested on the content-pro:r4.2.3-py3.11.9-ubuntu2204 image).

Base env list: env list base

Pro env list: env list pro

tnederlof commented 5 months ago

@aronatkins, I think enabling Quarto by default in the config is not breaking anything, but I wanted to check with you. Without it the environments all show Quarto as None by default.

aronatkins commented 5 months ago

Quarto is currently different from R/Python in that it can be enabled without a configured executable. At some point, we probably want all three R/Python/Quarto to behave more similarly (warn if enabled without executable).

tnederlof commented 5 months ago

@dbkegley I am nervous to put in hardcoded paths for Python and Quarto for non OHE since they need to be updated. When it was in PPM like that and they didn't get updated when the images changed, the charts would break. If the Connect team has a process to update when the images changed then it would make sense but want to make sure that is in place first.

dbkegley commented 5 months ago

If the Connect team has a process to update when the images changed then it would make sense but want to make sure that is in place first.

Appreciate the caution. I'll create a followup for the Connect team to find a way to flag this. I think having an GHA in this repo would be a nice addition to our pre-release testing

tnederlof commented 5 months ago

@dbkegley Okay I made the changes, now setting Python and Quarto directly in the default config section of the values.yaml and removed the conditional launcher only logic.

One thing I wanted to check on is whether the Executables defined for Quarto and Python are ignored in OHE (is there any harm in having them in there in the values?). Or do you think its cleaner to still have conditional logic where when in OHE we omit those Executable lines?

dbkegley commented 5 months ago

One thing I wanted to check on is whether the Executables defined for Quarto and Python are ignored in OHE (is there any harm in having them in there in the values?). Or do you think its cleaner to still have conditional logic where when in OHE we omit those Executable lines?

Good point. If OHE is enabled then any executables defined in the default values.yaml will be ignored so everything should work as expected, but it might confuse folks. Maybe we should add a comment in the values noting that these are the defaults for local execution only and the defaults for off-host are defined in the default runtimes.yaml

tnederlof commented 5 months ago

@dbkegley I changed the comments in values.yaml to make it clear that the executables do not apply for OHE and linked to the docs. I retested and things look good to me. I think this is ready when you want to merge (not sure who else should take a look).