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

Revert 138 revert 136 ovasdi/platform 5157/remove set output #141

Merged

Conversation

ovasdi
Copy link
Contributor

@ovasdi ovasdi commented May 22, 2023

This reverts the revert and includes a fix to address the typo in GITHUB environment variable.

Example of the GHA which didn't contain the typo succeeding this morning.

Copy link
Member

@dblandin dblandin left a comment

Choose a reason for hiding this comment

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

Nice catch! Left one minor suggestion but LGTM overall!

@@ -21,7 +21,7 @@ jobs:

- name: Run CLI
id: cli
run: echo "::set-output name=payload::$(artsy scheduled:next-on-call)"
run: echo "PAYLOAD=$(artsy scheduled:next-on-call)" >> $GITHUB_OUTPUT
Copy link
Member

@dblandin dblandin May 22, 2023

Choose a reason for hiding this comment

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

suggestion / non-blocking: Might be a good idea to double quote this environment variable name, just in case something changes internally that includes an unexpected space.

Suggested change
run: echo "PAYLOAD=$(artsy scheduled:next-on-call)" >> $GITHUB_OUTPUT
run: echo "PAYLOAD=$(artsy scheduled:next-on-call)" >> "$GITHUB_OUTPUT"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 9706b67

@dblandin dblandin merged commit b0ed8dd into main May 22, 2023
@dblandin dblandin deleted the revert-138-revert-136-ovasdi/PLATFORM-5157/remove-set-output branch May 22, 2023 15:37
@artsy-peril artsy-peril bot mentioned this pull request May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants