Skip to content
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 conda-store v0.3.10 and simplify specification of image #1130

Merged
merged 4 commits into from
Mar 1, 2022

Conversation

HarshCasper
Copy link
Contributor

Fixes #1126

@trallard trallard added area: integration/conda-store needs: review 👀 This PR is complete and ready for reviewing labels Mar 1, 2022
@danlester
Copy link
Contributor

danlester commented Mar 1, 2022

@HarshCasper Oh no, the image quansight/conda-store-server:v0.3.10 still appears in two places!

Sorry, I didn't realise that both conda-store-server and conda-store-worker use the same image... So I might have pointed you in the wrong direction.

Can you find another way to do all of this so that, essentially, the string quansight/conda-store-server:v0.3.10 (or related objects) appears only once?

There are many approaches you could take.

(1) Maybe reinstate all the Terraform variables but with no defaults in the Terraform variable definitions, and then pass through quansight/conda-store-server:v0.3.10 from input_vars.py as we did before.

(2) Or keep the string removed from input_vars.py and reinstate the Terraform variables plus a default only at one level - probably in conda-store.tf you would add the default {...} image setting, but make sure you do not also set the default in conda-store/variables.tf.

Or there are other possibilities. Personally I think (2) suits us best at this time - we will easily be able to override the image from Python code in the future if we want to.

In (2) this means you can't use the lowest level module (modules/kubernetes/services/conda-store or whatever as a folder) without explicitly passing through a conda-store-image value as we will do from conda-store.tf. But that's fine - the module is unlikely to be used separately from its surrounding codebase so we don't need to worry about it, and in any case whatever uses it can just pass through the value explicitly.

Copy link
Contributor

@danlester danlester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested as above.

@danlester
Copy link
Contributor

danlester commented Mar 1, 2022

Closes #1121

@HarshCasper HarshCasper merged commit 06e9402 into main Mar 1, 2022
@HarshCasper HarshCasper deleted the issue-1126 branch March 1, 2022 16:32
costrouc pushed a commit that referenced this pull request Mar 3, 2022
* remove conda store image from input_vars

* remove terraform variables

* uprade image to v0.3.10

* reinstate terraform variables with an image default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: integration/conda-store needs: review 👀 This PR is complete and ready for reviewing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade conda-store v0.3.10 and simplify specification of image
5 participants