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

artifact: enable inheriting environment variables from client #15514

Merged
merged 2 commits into from
Dec 9, 2022

Conversation

shoenig
Copy link
Member

@shoenig shoenig commented Dec 9, 2022

This PR adds client configuration for specifying environment variables that
should be inherited by the artifact sandbox process from the Nomad Client agent.

Most users should not need to set these values but the configuration is provided
to ensure backwards compatability. Configuration of go-getter should ideally be
done through the artifact block in a jobspec task.

e.g.

client {
  artifact {
    set_environment_variables = "TMPDIR,GIT_SSH_OPTS"
  }
}

Closes #15497

@shoenig shoenig force-pushed the f-getter-environment branch from e2b5d8a to be57726 Compare December 9, 2022 17:59
@shoenig shoenig changed the title [no ci] artifact: enable inheriting environment variables from client artifact: enable inheriting environment variables from client Dec 9, 2022
@shoenig shoenig added this to the 1.5.0 milestone Dec 9, 2022
This PR adds client configuration for specifying environment variables that
should be inherited by the artifact sandbox process from the Nomad Client agent.

Most users should not need to set these values but the configuration is provided
to ensure backwards compatability. Configuration of go-getter should ideally be
done through the artifact block in a jobspec task.

e.g.

```hcl
client {
  artifact {
    set_environment_variables = "TMPDIR,GIT_SSH_OPTS"
  }
}
```

Closes #15498
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

LGTM! I can bikeshed the name but I'm happy with it as-is as well 😀

Comment on lines +43 to +46
// SetEnvironmentVariables is a comma-separated list of environment
// variable names to inherit from the Nomad Client and set in the artifact
// download sandbox process.
SetEnvironmentVariables *string `hcl:"set_environment_variables"`
Copy link
Member

Choose a reason for hiding this comment

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

On the name "set": is there future in which we might want to let users set specific env vars, rather than having them inherit from the client?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can continue using this field to support setting custom values, just by interpreting '=' as key+value, e.g.

set_environment_variables = "BLAH,PATH=/opt/custom/bin"

But that would be beyond compatibility support, so that's a PR for another day

Comment on lines 385 to 387
- `set_environment_variables` `(string:"")` - Specifies a comma separated list
of environment variables that should be inherited by the artifact sandbox from
the Nomad client's environment.
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to call out the default variables here so that people don't think they need to add PATH?

@github-actions
Copy link

github-actions bot commented Apr 9, 2023

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

artifact: enable setting custom environment variables in artifact downloader sandbox
2 participants