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

Don't inherit environment variables from daemon process #297

Merged
merged 1 commit into from
Mar 4, 2022

Conversation

drewkett
Copy link
Contributor

@drewkett drewkett commented Mar 4, 2022

Currently, the environment variables associated with a task are added to those of the daemon process on launch. Use env_clear() to start with a clean environment before adding the task environment variables

I wasn't sure if this should merge to main or development

Checklist

  • I picked the correct source and target branch.
  • I included a new entry to the CHANGELOG.md.
  • I checked cargo clippy and cargo fmt. The CI will fail otherwise anyway.
  • (If applicable) I added tests for this feature or adjusted existing tests.
  • (If applicable) I checked if anything in the wiki needs to be changed.

Currently, the environment variables associated with a task are added
to those of the daemon process on launch. Use `env_clear()` to start
with a clean environment before adding the task environment variables
@Nukesor
Copy link
Owner

Nukesor commented Mar 4, 2022

I would consider this a bug, so a merge against main is correct :)

@Nukesor
Copy link
Owner

Nukesor commented Mar 4, 2022

Good catch, I didn't expect the environment variables to be automatically inherited, but I guess it makes sense that this is the default behavior.

Thanks for discovering and fixing this :)

I want to write an integration test before merging this. Once I've done this and it fails as expected on the current main, I'll merge your PR :D. You don't have to do anything on your own, but this definitely is something that should be properly tested.

@drewkett
Copy link
Contributor Author

drewkett commented Mar 4, 2022

Related to this, I had the thought was whether the environment variables should also be set for callbacks. I didn't actually test them but the code seemed to indicate that they are not set.

@Nukesor
Copy link
Owner

Nukesor commented Mar 4, 2022

Nope they're not set yet.

What's a good use-case for this?

@drewkett
Copy link
Contributor Author

drewkett commented Mar 4, 2022

Honestly I don't have one. I ran into this original issue cause I had set an environment variable in the terminal that I then used to launch pueued. And that caused an error in a task since it tried to use that environment variable but it was incorrect. So my thinking with the callback is that the more correct environment for the callback might be the environment of the task versus whatever the environment is for the daemon.

Edit: thinking about it more, I think I was thinking about callbacks wrong and I think a consistent environment (ie the daemon one) for them probably makes the most sense.

@Nukesor
Copy link
Owner

Nukesor commented Mar 4, 2022

Perfect :)

I think the daemon environment is good enough for callbacks for now. Let's keep it that way until somebody runs into some problems and comes up with a proper use-case for using a different environment.

Until then, thanks for your fix! I managed to write a proper integration test for this bug and your change indeed fixes the problem at hand.

@Nukesor Nukesor merged commit 182d24c into Nukesor:main Mar 4, 2022
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.

2 participants