-
Notifications
You must be signed in to change notification settings - Fork 557
Fix syntax for environment variable exports #2636
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
Conversation
run: | | ||
echo "export KUBERNETES_BRANCH=${{ github.event.inputs.kubernetesBranch }} >> ./settings" | ||
echo "export GEN_COMMIT="${{ github.event.inputs.genCommit }}" >> ./settings" | ||
echo "export KUBERNETES_BRANCH="${{ github.event.inputs.kubernetesBranch }}"" >> ./settings |
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.
Is it possible to mix single and double quotes on these lines? Or, maybe some of the quotes could be escaped? Right now, it's kind of confusing. I think you're intending to add quotes around the value of KUBERNETES_BRANCH
, but it seems like the first added quote would match the quote before export
and the second set of quotes would be an empty string. Maybe I'm misunderstanding the intention though?
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.
Single quotes for the branch name would be better as this wouldn't result in shell expanding when we would have some fancy branch names.
I'm not 100% sure if GitHub-actions would replace that value in single quotes, but I guess so:
echo "export KUBERNETES_BRANCH="${{ github.event.inputs.kubernetesBranch }}"" >> ./settings | |
echo "export KUBERNETES_BRANCH='${{ github.event.inputs.kubernetesBranch }}'" >> ./settings |
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.
Actually the real thing I'm trying to do is get rid of the end quote at settings"
which was actually what was causing the issue. I think I could eliminate the interior quotes all together unless version is going to have spaces in it, which seems unlikely.
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.
Seems fine to me.
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.
Go ahead, sounds fine to me as well.
Addressed comment, please take another look. |
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!
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, cjihrig The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
No description provided.