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

Pass service description's environment variables to child process #18

Merged
merged 3 commits into from
Mar 23, 2021

Conversation

benhoyt
Copy link
Contributor

@benhoyt benhoyt commented Mar 21, 2021

Pretty self-explanatory. Fixes #17

Copy link
Member

@hpidcock hpidcock left a comment

Choose a reason for hiding this comment

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

LGTM thank-you

@hpidcock
Copy link
Member

@benhoyt @niemeyer would also be good to do env var expansion here so someone could do

environment:
   - PATH: $PATH:/my/bin/dir

@benhoyt
Copy link
Contributor Author

benhoyt commented Mar 22, 2021

@benhoyt @niemeyer would also be good to do env var expansion here so someone could do PATH: $PATH:/my/bin/dir

Yeah, I agree, though I think we can leave that for a subsequent PR (this is strictly the bug fix).

@niemeyer
Copy link
Contributor

@hpidcock @benhoyt Agreed, on both counts. We should do it on a follow up, but we shouldn't wait too much as that may be seen as breaking compatibility. Note that this is the key reason why we have that as a list and note as a map: ordering matters if we want to expand.

cmd.Env = os.Environ()
for _, v := range service.Environment {
cmd.Env = append(cmd.Env, v.Name+"="+v.Value)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing this, Ben. We indeed need a good test for this sort of change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, and it's also worth checking (and testing) what happens if there's a variable in the environment process which needs to be replaced by something explicit on the service.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a test for this: it tests brand new env vars, env vars only set in the parent, and env vars in the parent but overridden in the layer config. I'll do any $PATH type of interpolation in a subsequent commit.

Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Thanks!

command: /bin/sh -c "env | grep PEBBLE_ENV_TEST | sort > %s; sleep 300"
environment:
- PEBBLE_ENV_TEST_1: foo
- PEBBLE_ENV_TEST_2: bar bazz
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice test.

@benhoyt benhoyt merged commit a342556 into canonical:master Mar 23, 2021
@benhoyt benhoyt deleted the fix-env branch March 23, 2021 20:01
jujubot added a commit to juju/juju that referenced this pull request Mar 23, 2021
#12804

This includes the following Pebble fixes:

* canonical/pebble#18 - Pass service description's environment variables to child process
* canonical/pebble#20 - Change default: start/stop to startup: enabled/disabled
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.

Pebble not passing configured environment variables to services
3 participants