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

refactor: adjust PR Title Check workflow to reduce noise #12373

Merged
merged 6 commits into from
Aug 13, 2024

Conversation

BigLep
Copy link
Member

@BigLep BigLep commented Aug 9, 2024

Edited by Orjan:

This PR originally intended to accumulate release flow improvements, but is now repurposed to address issues with the PR Title Check. The current implementation is causing excessive noise on draft PRs, as seen in #12322. This change aims to reduce unnecessary notifications while maintaining the check's functionality.

This is accumulating improvements based on lessons learned from executing #12343
@BigLep BigLep added the skip/changelog This change does not require CHANGELOG.md update label Aug 9, 2024
@BigLep BigLep mentioned this pull request Aug 9, 2024
37 tasks
@BigLep BigLep changed the title docs: update RELEASE_ISSUE_TEMPLATE.md base on v1.29.0 release execution docs(v1.29.0): release template improvements Aug 9, 2024
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

fix(ci): allow periods in PR title scope
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

@rjan90
Copy link
Contributor

rjan90 commented Aug 11, 2024

Seems like we found an edge-case in the pr-title-check, where the regex pattern doesn't account/allow for periods (.) in the scope. I pushed a change here: de60b08. To confirm it fixes it.

@rvagg
Copy link
Member

rvagg commented Aug 12, 2024

I think I'd blame the message rather than the regex here, it probably should be something like a bare docs:, like docs: v1.29.0 release template improvements, or docs(release):, like `docs(release): template improvements from v1.29.0

@rjan90 rjan90 changed the title docs(v1.29.0): release template improvements docs: release template improvements Aug 12, 2024
@github-actions github-actions bot dismissed their stale review August 12, 2024 06:08

PR title now matches the required format.

@rjan90 rjan90 marked this pull request as ready for review August 12, 2024 06:32
Allows GitHub to properly register the review, addressing the issue where the review wasn't being dismissed despite the PR title being updated.
@github-actions github-actions bot dismissed their stale review August 12, 2024 08:10

PR title now matches the required format.

PR title check workflow to dismiss review only on title edit
@rjan90 rjan90 changed the title docs: release template improvements refactor: adjust PR Title Check workflow to reduce noise Aug 12, 2024
@rjan90
Copy link
Contributor

rjan90 commented Aug 12, 2024

@BigLep Sorry that I kind of hijacked this PR to refactor the PR Title Check workflow to reduce noise, as I got bothered by it when working on the UPDATE_RELEASE_FLOW draft PR: #12322.

In hindsight I should have opened a separate PR for this, but the monday "jetlag" got the better of me! I will open a new PR for accumulating improvements based on lessons learned from executing #12343 once this lands.

@BigLep
Copy link
Member Author

BigLep commented Aug 12, 2024

I think I'd blame the message rather than the regex here, it probably should be something like a bare docs:, like docs: v1.29.0 release template improvements, or docs(release):, like `docs(release): template improvements from v1.29.0

That's fare. I'll adjust to docs(release): and put the version number in the description itself.

@BigLep
Copy link
Member Author

BigLep commented Aug 12, 2024

Sorry that I kind of hijacked this PR to refactor the PR Title Check workflow to reduce noise

No problem - I'll create a new PR with the release issue improvements. (I now need to also add the change to doing "docs(release): ..."

Copy link
Member Author

@BigLep BigLep left a comment

Choose a reason for hiding this comment

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

While we're here, how about we remove "synchronize" from line 5. Pushing commits shouldn't need to trigger this workflow.

Also, given we've done a few rounds of tweaking this, I'm wondering if we would have been better off going with something off the shelf that has already been tweaked...

@rjan90
Copy link
Contributor

rjan90 commented Aug 12, 2024

Also, given we've done a few rounds of tweaking this, I'm wondering if we would have been better off going with something off the shelf that has already been tweaked...

I agree. What we potentially miss with going with something off the shelf, is the period where both styles are allowed (i.e fix(releases): and fix: releases:)

If we encounter more issues with it, I´m inclined to go with the off the shelf option, just so that we do not spend more time on fixing it.

@BigLep
Copy link
Member Author

BigLep commented Aug 12, 2024

What we potentially miss with going with something off the shelf, is the period where both styles are allowed (i.e fix(releases): and fix: releases:)

The top three tools I saw all allow passing in a custom regex. That said, looking at it more, I don't think other tools meet all our needs. I just added a comment in the original PR: #12340 (comment)

So, basically lets hold the course and finish fine tuning this for our needs. Thanks!

@BigLep
Copy link
Member Author

BigLep commented Aug 12, 2024

New PR for doing the release process improvements: #12378

@rjan90 rjan90 merged commit a86129b into master Aug 13, 2024
87 checks passed
@rjan90 rjan90 deleted the docs/release-issue-template-updates-for-v1.29.0 branch August 13, 2024 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip/changelog This change does not require CHANGELOG.md update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants