-
Notifications
You must be signed in to change notification settings - Fork 144
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
feat: add environment-variables support #114
Conversation
Any news about this PR? |
+1 looking forward to this one |
Just waiting for a review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Misterio77 !
Looks great, apologies for the late review 🙇 some tiny suggestions below and then we can merge it!
action.yml
Outdated
environment-variables: | ||
description: 'Variables to add to the container. In the form "NAME=value|FOO=bar"' | ||
required: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a very naive question 😅
But is it possible for environment-variables
to be a map instead of a string?
Something like:
environment-variables:
NAME: value
FOO: bar
Assuming that's not possible, can we use comma "," as a separator instead of "|" that seems to be more common to split values?
environment-variables: "BUCKET=my-bucket,KEY=file.txt"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly the inputs are always strings, as far as i know: https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions#inputs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About the separator, i've seen a couple cases where environment variables contents might include commas, so i choose the pipe (which is not used, as to avoid problems with accidentally bash piping stuff) instead.
Maybe there's some way we can allow the users to escape the commas? But i personally think using pipes is easier and does not require the user to scan their variables looking for rogue commas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha thanks the explanation makes sense! I'm still reluctant to use pipes as separator as it's so unusual 🤔
One other alternative, that I can think of is instead splitting on new lines like this if users want to define multiple:
environment-variables: |
BUCKETS=my-bucket,my-other-bucket
KEY=file.txt
https://git.luolix.topmunity/t/can-action-inputs-be-arrays/16457/2 which seems to be a common approach with GH actions, and aligns well with the syntax of environment files.
We can then in the code do this: s.split('\n').map((line) => line.trim()).filter((pair) => !!pair)
Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great idea, i think it looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we can (or should) write multi-line descriptions on action.yml
, so i've documented the change, but without a "literal example".
Please let me know if it looks okay :))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, ty for the quick follow-ups! Looks great, I suggested some small wording change here: #114 (comment)
I think we're almost done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the reviews!
No worries at all about the delay! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! One last change sorry 🥺 and if we can fix the merge conflicts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woohoo!🥳
All squashed and rebased! |
Yay merged! Sorry it took a while because I don't have write permissions, so I had to chase after someone that could merge it :) |
No problem at all! Thanks a lot |
Hey guys, thanks for this rich contribution. @Misterio77 @efekarakus <3 but this new feature wasn't released yet, right? The code was merged, but no one new version was released. I am just making a reminder, cause using the Commit Id Works fine! Thanks again. |
can confirm it is missing but thx to @robsonalves it works well with commit id
|
* feat: support for setting environment variables * chore: Update dist Co-authored-by: GitHub Actions <runner@fv-az224-488.bi4zmy1qo3tuniz3adueuvvcza.cx.internal.cloudapp.net>
* feat: support for setting environment variables * chore: Update dist Co-authored-by: GitHub Actions <runner@fv-az224-488.bi4zmy1qo3tuniz3adueuvvcza.cx.internal.cloudapp.net>
Issue #, if available: #112
Description of changes: These commits add a
environment-variables
input. It acceptsKEY=value
environment variables, separated by|
.For example:
VARIABLE_NAME=value|ANOTHER_ONE=foo|YET_ANOTHER=bar
.Those are added to the rendered task definition: if the
environment
array does not exist, it is created. if the specifiedKEY
name is present it is updated, if not, created as well.I've updated the tests to include testing for all these cases (creating array, update existing, create new, not touching one that is already there, etc), all is missing currently is a test case for the new throw (when the input has an invalid format).
Here's an usage example, to update a container environment variable with the deployed tag (my use case requires that variable to return, at request, the currently running service vesion, which otherwise would not be accessible at run-time):
Following semver, i've bumped the version on
package.json
to1.1.0
, as a new feature was added in a backward-compatible way.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.