-
Notifications
You must be signed in to change notification settings - Fork 1
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
ci: fix publish step check before tagging #39
Conversation
if: ${{ steps.release.outputs.release_created }} | ||
# https://github.com/googleapis/release-please-action?tab=readme-ov-file#creating-majorminor-tags | ||
- name: Tag versions | ||
if: ${{ steps.release.outputs.release_created && fromJSON(steps.publish-crates.outputs.published).* }} | ||
if: ${{ steps.release.outputs.release_created && steps.publish-crates.outcome == 'success' }} |
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.
the stringified version of an empty array in JS is just an empty string (not counting however @actions/core decides to serialize the value).
given error message from the last successful release, it made it seem like the value was an empty string (Path '', line 0, position 0.
) so i wasn't too confident in just checking that.
a failure on the publish step should result in a step failure based on https://github.com/katyo/publish-crates/blob/5e67639f17e8a1f221e804c2bd47f7a253e45dac/src/main.ts#L145
so i figured we could just check for the success of the publish step instead.
@@ -26,10 +26,11 @@ jobs: | |||
- uses: katyo/publish-crates@v2 | |||
with: | |||
registry-token: ${{ secrets.CARGO_REGISTRY_TOKEN }} | |||
id: publish-crates |
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.
Did you get a chance to test if this line alone fixes the error? Running actionlint on the main branch flags the missing id
for this GitHub Actions step, as it’s used later but isn’t currently defined.
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 did try to check it, but the output was always an empty string, so i couldn't be sure if it would've worked or not with an actual release.
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.
The test run here shows fromJSON
works when the output includes the array string for a new version. However, if there’s no new version, the output is an empty string, causing fromJSON
to fail.
The outcome == 'success'
check should work well, but an empty string check could also suffice.
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 think i'll opt to leave it with `outcome == 'success' since it's a bit easier to understand without looking through the action's docs for someone else on our team to work on this in the future. but good to see the output on a successful run as well
What's New?
fromJSON
was unable to parse the value ofsteps.publish-crates.outputs.published
, so this change uses an alternative value to check for publication success.Screenshots (if appropriate):
Type of change
Checklist:
Additional context