Skip to content
This repository has been archived by the owner on Jul 25, 2024. It is now read-only.

in setup.sh, setting env vars via --update-env-vars is not always idempotent, it fails if a variable doesn't exist #844

Closed
mco-gh opened this issue Apr 10, 2023 · 2 comments
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@mco-gh
Copy link
Contributor

mco-gh commented Apr 10, 2023

Ran into this problem: open-telemetry/opentelemetry-operator#1148.
My prod deployment was fine but my staging site wasn't working. Turns out it was due to the setup.sh script using –update-env-vars instead of –set-env-vars so if you don’t have a certain required variable set in a Cloud Run deployment (which I didn’t for my staging area, though my prod project had it) then you won’t get that variable set. It should probably prefer –set-env-vars to set the value unconditionally.

Chat discussion followed...

Adam Ross, Yesterday 19:18
Just a quick response to chat, my understanding of update vs. set is that set clears all unspecified variables and update only modifies variables as named in the command. What you describe sounds like a different behavior

Marc Cohen, Yesterday 20:25, Edited
I saw that set wipes everything so I think what you want to do is a compound set, setting all the vars in one option (like you do already somewhere else, I think it's SITE_PARAMS, or something like that).

Marc Cohen, Yesterday 20:26, Edited
I suppose wiping other vars could be bad though, if the end user added some custom variables. Never mind. 🙂

I mean for someone who built on top of emblem.

Adam Ross, 22 min
, Edited
I think there's still a need to find the cause somewhere here, because update and set are equally unconditional. The only difference is that set wipes unspecified vars.

Adam Ross, 20 min

Filing an issue is welcome.

Marc Cohen, 3 min

will do

@mco-gh mco-gh added priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Apr 10, 2023
@grayside grayside added priority: p2 Moderately-important priority. Fix may not be included in next release. and removed priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. labels Apr 13, 2023
@grayside
Copy link
Collaborator

Next step: Reproduce the issue by examining an end-to-end Emblem setup deploy.

@glasnt
Copy link
Contributor

glasnt commented Jun 30, 2023

setup.sh should only be run once, on new projects; the difference between --update-env-vars and --set-env-vars on creation of services should be identical.

If there's existing services in an environment, resetting configurations through the setup.sh script isn't in scope. Reapplying terraform to reassert state, but emblem chooses to use scripts for the cloud run services.

To make a more idempotent environment, it would be a feature/refactor to move all setup into terraform.

(I would now close this issue, but I don't have permissions)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

4 participants