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

Add ability to provide env vars to plugins #5359

Merged
merged 5 commits into from
Sep 20, 2018
Merged

Conversation

calvn
Copy link
Member

@calvn calvn commented Sep 19, 2018

Fixes #5177

We could add a field of allowed pass-through environment variables to the plugin catalog. Then you can specify which ones should be passed through to the child process. Or, we could allow directly specifying key-value pairs. Or both (e.g. we could have passthrough_env and additional_env or so).

Only the former of the two options is implemented. The reasoning is the same behind separating the command and args params, which started off as being one and the same before getting split (and tagging the 2-in-1 option as deprecated). If users end up asking for the ability to pass in key=value pairs as part of the command then we can add it later, but this is more in line with how exec.Command works.

briankassouf
briankassouf previously approved these changes Sep 19, 2018
Copy link
Member

@briankassouf briankassouf left a comment

Choose a reason for hiding this comment

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

👍 looks great! Can you also add the docs changes to this PR?

@briankassouf briankassouf added this to the 0.11.2 milestone Sep 19, 2018
@calvn
Copy link
Member Author

calvn commented Sep 19, 2018

Sure thing! I also plan on refactoring/combining TestAddTestPlugin and TestAddTestPluginTempDir before merging this PR.

Copy link
Member

@jefferai jefferai left a comment

Choose a reason for hiding this comment

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

Looks great!

@calvn calvn merged commit 494b9a0 into master Sep 20, 2018
@calvn calvn deleted the plugin-catalog-env-param branch September 20, 2018 17:50
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.

3 participants