-
Notifications
You must be signed in to change notification settings - Fork 55
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
Add $VAR-style interpolation into environment variable handling #182
Comments
Note for future reference: in craft and spread, there is already a notation for handling variables. E.g. |
We should also consider enabling this in
|
@cjdcordeiro What does the craft/spread notation |
@benhoyt the syntax is I've left that comment above as I think it might be worth considering a similar implementation here. Maybe that's overkill, but worth considering nonetheless. I could imagine something like: services:
bind9:
override: replace
command: app1 $(othersvc: echo $FOO)
othersvc:
override: replace
command: app2
environment:
FOO: bar2 P.S. if simply allowing interpolation for |
My 5 cents: I've given some initial thought to it, and I find this example a bit confusing. The Confusing PartWhen I see this config, to me,
To me, the definition is equivalent to: if [[ -n $BAR ]]; then
FOO="The $BAR bazzes."
else
FOO="The bazzes."
fi So, if: export user="tiexin"
export BAR="some alue" I'd translate the layer config to: variables:
bar: "some value"
services:
svc:
override: replace
command: sleep 1000
environment:
THINGDIR: /home/tiexin/thing
FOO: "The some value bazzes."
BAR: "bar" Then I continued to read the line I think the ambiguity comes from the fact that Possible SolutionTo solve this, we need to introduce the concept of variables. If we can define something like: variables:
bar: "some value"
services:
svc:
override: replace
command: sleep 1000
environment:
THINGDIR: /home/${USER}/thing
FOO: "The var.bar bazzes."
BAR: "var.bar" Then it's crystal clear that:
Except we can't use So, maybe variables:
bar: "some value"
services:
svc:
override: replace
command: sleep 1000
environment:
THINGDIR: /home/${USER}/thing
FOO: "The [[ var.bar ]] bazzes."
BAR: "[[ var.bar ]]" Which is exactly how GitHub Actions and Terraform do it. Final ThoughtAdding support to env vars and variables might be an overkill, though. Is there a concrete example of which charm needs this feature? In my opinion, only adding support to env var is more than enough. services:
svc:
override: replace
command: sleep 1000
environment:
THINGDIR: /home/${USER}/thing
FOO: "The some value bazzes."
BAR: "some value" This is more than OK IMHO, although it has some duplicated content, but is there a concrete example of having lots of duplicated values in the layer config and causing troubles? |
@IronCore864 and I discussed this briefly today. I personally don't find it confusing: they're all environment variables, it's just whether they come from the parent (Pebble) environment or the current environment being defined in the configuration layer. I've updated the example to be a bit more realistic, however, and the ordering to be more obvious. Kubernetes uses similar syntax, though it uses parentheses instead of curly brackets (and it looks like only "current environment" is in play here. Still, I don't think the original proposal is confusing, and I think we should stick with it. Go's @IronCore864 is going to create a short (1-2 page) spec officially proposing this as the next spec. |
We also have to consider:
|
PoC: https://github.com/IronCore864/yaml-config-env-interpolation/tree/main Will do a simple spec later. |
Link to spec OP048 for reference. |
A while ago we discussed (in a discussion about whether environment variables should be an ordered list or a map - be we settled on a map for simplicity) that it would be good to have a way to insert other environment variables into the definition for new variables. For example:
We could use
os.Expand
for this. Though we'd have to be careful about doing them in the correct order (topological sort) to resolve dependencies correctly, like the one fromCMD_SOCKET
toCMD_DIR
above.This is similar to the Kubernetes environment variable feature.
The text was updated successfully, but these errors were encountered: