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

NR-249371: reflect failures in pre-release title #60

Merged
merged 3 commits into from
Apr 9, 2024

Conversation

sigilioso
Copy link
Contributor

@sigilioso sigilioso commented Apr 5, 2024

This PR edits pre-release titles to reflect the corresponding artifacts states:

State Title pre-release
pre-release created vX.Y.Z (artifacts-pending) true
pre-release ready vX.Y.Z true
pre-release failure vX.Y.Z (pre-release-failure) true

This should help us to easily identify failing pre-releases both manually and automatically.

Out of scope:

Since releases are not promoted automatically (yet), the release workflow is not modified. The release title should also reflect failures when promoting from pre-releases to releases automatically.

TODO:

  • Move the latest tag when the changes are merged.

@sigilioso sigilioso requested a review from a team April 5, 2024 08:16
@paologallinaharbur
Copy link
Member

I am not sure about the approach. How can differentiate a pre-release that succeeded from one that was not executed?
In this case if we had a failure with permissions we would not be able to attach the info.

Would make sense to add more structured info in the description itself?

status: created
status: pre-released
status: released
status: pre-released-failed
status: released-failed

@sigilioso
Copy link
Contributor Author

We cannot differentiate them, I was considering the failing use-case only. Thanks for pointing it out.

I wanted to keep it simple but maybe it is not possible, it seems we definitely need more than two values. I'll keep working on it.

@sigilioso
Copy link
Contributor Author

sigilioso commented Apr 5, 2024

Considering that there is already a flag to say that the release is a pre-releae (and we are using it), and pre-released and released are the expected statuses. Maybe we can simplify the status definition a bit and keep using the title.

Then, we could use:

State Title pre-release
created vX.Y.Z (artifacts-pending) true
pre-release vX.Y.Z true
release vX.Y.Z false
pre-release-fail vX.Y.Z (pre-release-failure) true
release-fail vX.Y.Z (release-failure) false

WDYT?

@paologallinaharbur
Copy link
Member

I like it!

@sigilioso sigilioso force-pushed the NR-249371-identify-failing-pre-releases branch from ec2c592 to 06af967 Compare April 8, 2024 06:46
@sigilioso
Copy link
Contributor Author

I've just updated the workflows and the PR description, @paologallinaharbur take a look when you have a while.

Comment on lines +233 to +238
- uses: actions/checkout@v4
- name: Reflect failure in release title
env:
GH_TOKEN: "${{ secrets.COREINT_BOT_TOKEN }}"
run: |
gh release edit ${{ inputs.tag }} --title "${{ inputs.tag }} (pre-release-failure)"
Copy link
Member

Choose a reason for hiding this comment

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

I would split notify a failure and this to increase the changes to have at lease one executed is something is broken

Copy link
Member

Choose a reason for hiding this comment

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

two diffrent jobs I mean, both having ${{ always() && failure() }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified here: 3e9a018

Copy link
Member

@paologallinaharbur paologallinaharbur left a comment

Choose a reason for hiding this comment

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

apart from that NIT, it is good to go 😄

Copy link
Contributor

@marcsanmi marcsanmi left a comment

Choose a reason for hiding this comment

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

LGTM! 👍
Should we update the scripts added in this PR #59 to reflect these changes?

@sigilioso
Copy link
Contributor Author

sigilioso commented Apr 9, 2024

LGTM! 👍 Should we update the scripts added in this PR #59 to reflect these changes?

I've just updated the one listing pre-releases in #61, I didn't want to simplify it because getting the failing job links is also valuable

@sigilioso sigilioso merged commit e57d2db into main Apr 9, 2024
5 checks passed
@sigilioso sigilioso deleted the NR-249371-identify-failing-pre-releases branch April 9, 2024 06:53
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.

3 participants