enable steps to define environment variables #1918
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
devigned
requested review from
carolynvs,
jeremyrickard and
vdice
as code owners
devigned
force-pushed
the
env-step-vars
branch
2 times, most recently
from
575f4d0 to
120cf77
Compare
carolynvs
requested changes
Feb 22, 2022
There was a problem hiding this comment.
This looks great! Just a couple things need to be addressed first:
- main is for our stable release, v0.38, and we are only merging high priority patches into that branch. Pull requests with new features should target release/v1.
- This adds support for allowing mixins to set environment variables but at this point a user couldn't actually try out the feature yet. What we normally do is have the exec mixin implement new features added to the builder package, demonstrating how it works and vetting the implementation.
- We also need documentation showing how it works, this is usually what the mixin authors will copy when they look at the exec mixin and incorporate the changes necessary to support a new feature (like setting env vars). Here's the exec mixin page that should be updated to include how to set an env var in its supported syntax section.
carolynvs
approved these changes
Mar 3, 2022
| _, err := ExecuteStep(c.Context, step) | ||
| require.NoError(t, err, "Execute Step failed") | ||
| containsEnv := strings.Contains(c.GetOutput(), "SOME_VAR_123=foo") | ||
| // use assert.True rather than assert.Contains so that the env vars are not all sent to the test output. There might |
There was a problem hiding this comment.
I hadn't considered that, good call! 👍
Signed-off-by: David Justice <[email protected]>
carolynvs
approved these changes
Mar 3, 2022
| @@ -72,6 +72,13 @@ | |||
| "type": "string" | |||
| } | |||
| }, | |||
| "envs": { | |||
There was a problem hiding this comment.
A million brownie points for remembering to update the schema that VS Code uses to do autocomplete 💯
|
You can fix the broken unit test by running |
Yep. Thank you for having the prompt in test failure output. That was super helpful. |
|
/azp run porter-integration |
|
Commenter does not have sufficient privileges for PR 1918 in repo getporter/porter |
Signed-off-by: David Justice <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this change
This change add the ability for steps to add environment variables to be added to the env when executing the command for a step.
This change is related to getporter/az-mixin#38. With this change, the mixin could add the user-agent env var to the command.
Notes for the reviewer
This is my first contribution here, so please let me know if I missed / misunderstood something.
Checklist
Reviewer Checklist