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 extra_env_vars field to experimental_shell_command target. #15742

Merged

Conversation

danxmoran
Copy link
Contributor

@danxmoran danxmoran commented Jun 3, 2022

Closes #15728

[ci skip-rust]
[ci skip-build-wheels]

@@ -324,6 +335,7 @@ class ShellCommandTarget(Target):
ShellCommandSourcesField,
ShellCommandTimeoutField,
ShellCommandToolsField,
ShellCommandExtraEnvVarsField,
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 this to the end but looks like it should maybe be put in alphabetical order? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I'm one in favor of sorting lists alphabetically, however here I think the order affects the order the fields are presented in the documentation so that is worth considering too.

@@ -162,6 +164,9 @@ async def prepare_shell_command_process(
rationale=f"execute `{shell_command.alias}` {shell_command.address}",
)

extra_env = await Get(Environment, EnvironmentRequest(extra_env_vars))
command_env.update(extra_env)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried setting command_env = await Get(Environment, EnvironmentRequest(extra_env_vars)) from the start, but Environment is frozen so it doesn't allow the follow-up additions needed to set the tools path

Copy link
Member

@stuhood stuhood 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! Thank you.

Comment on lines +306 to +312
help = softwrap(
"""
Additional environment variables to include in the shell process.
Entries are strings in the form `ENV_VAR=value` to use explicitly; or just
`ENV_VAR` to copy the value of a variable in Pants's own environment.
"""
)
Copy link
Member

Choose a reason for hiding this comment

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

It's probably worth extracting this help string to a templating function, maybe in src/python/pants/engine/environment.py ? It's duplicated in at least src/python/pants/backend/python/target_types.py and src/python/pants/core/goals/test.py.

Copy link
Member

Choose a reason for hiding this comment

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

Oh... or defining a Field superclass that provides it? Maybe overkill.

Copy link
Member

Choose a reason for hiding this comment

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

Might be done as a follow up as well.

Copy link
Member

@kaos kaos left a comment

Choose a reason for hiding this comment

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

Sweet 😄

@stuhood stuhood enabled auto-merge (squash) June 3, 2022 18:19
@stuhood stuhood merged commit 77d6b68 into pantsbuild:main Jun 3, 2022
@danxmoran danxmoran deleted the danmoran/experimental-shell-env-vars branch June 3, 2022 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow setting/passing through env vars to experimental_shell_command
3 participants