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

reduce environment variable provider precedence #2886

Conversation

kate-goldenring
Copy link
Contributor

Inverts precedence of variables providers giving the environment variable provider least precedence

fixes #2584

- inverts precedence of variables providers giving the environment variable
provider least precedence

Signed-off-by: Kate Goldenring <[email protected]>
Copy link
Collaborator

@rylev rylev left a comment

Choose a reason for hiding this comment

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

This is fine from a technical point of view. As for whether this is the right behavior or not, I'm not sure. I'm curious if @itowlson still has concerns about this.

@kate-goldenring
Copy link
Contributor Author

I agree that I would like @itowlson's thoughts before merging. I understand that env vars seem like they should take precedence on the CLI but seem less relevant when orchestrating Spin apps with SpinKube. Maybe we should make this configurable by the runtime when it loads the variables factor.

@itowlson
Copy link
Contributor

Yes, I still have concerns, but by my count the majority of people feel this is the right thing to do. And my concern is for a specific scenario (the override on the command line one) that, at a technical level, is impossible to differentiate from other scenarios where I do agree this is the right thing to do. So I am keeping mousy quiet.

What? By my standards, 60 words is mousy quiet.

@kate-goldenring kate-goldenring merged commit 2a9bf7c into fermyon:main Oct 17, 2024
17 checks passed
@kate-goldenring kate-goldenring deleted the invert-variables-provider-precedence branch October 17, 2024 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider deprioritizing the environment variable provider
5 participants