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

Add new steps to labelPR job to remove MnR label if version.env has been manually edited #28

Merged
merged 8 commits into from
Jan 29, 2025

Conversation

angelowilliams
Copy link
Contributor

@angelowilliams angelowilliams commented Jan 14, 2025

Dependencies

N/A.

Documentation

Description

Awhile back ago, I was working on a major version upgrade to the list service. I probably thought to myself early on, "I know this is going to be a major version upgrade, so let me just update version.env now" (the validate-semver job probably yelled at me). Then, a week passed, maybe a weekend too, and it was time to deploy my PR. Forgetting that I had already updated the versioning myself, I used the merge-and-release extension to do another major version bump. In the case of minor or patch versions, this is no big deal, but for a major version it definitely created an extra headache. I'm sure that I'm not the only person who has done this before, I might've just been the first one to do this with a major version where it's noticeable.

This PR prevents this behavior by changing the labelPR job to remove the merge-and-release-compatible label if:

  1. The label has already been attached to the PR.
  2. version.env has been manually updated.

In order to adequately catch version.env updates, the labelPR would now run on not just PR opened events, but also anytime a commit was pushed. Thus, if you updated version.env in a commit after already opening the PR, the label would be removed. If you then made another commit reverting your version.env changes, the label would be re-added.

Testing Considerations

Here are the following test cases I investigated:

label already exists? version.env already changed? expected result workflow run link
false false label added https://github.com/nicheinc/gha-test-environment/actions/runs/12777619519/job/35660486355
false true nothing https://github.com/nicheinc/gha-test-environment/actions/runs/12792470788/job/35663025133
true false label not removed https://github.com/nicheinc/gha-test-environment/actions/runs/12792359936/job/35662644096
true true label removed https://github.com/nicheinc/gha-test-environment/actions/runs/12792402327/job/35662788153

I also did a deployment of the test PR linked in the table above, everything went as normal.

Note that all my tests were in a BE repo. I don't know why behavior would be different in a FE repo, but keep that limitation in mind.

Deployment

Merge to main.

Versioning

This workflow has no versioning.

@angelowilliams angelowilliams marked this pull request as draft January 14, 2025 22:24
@angelowilliams angelowilliams marked this pull request as ready for review January 15, 2025 16:59
@@ -30,15 +30,51 @@ on:
jobs:
#label PR as being compatible with the `update-versions` command
labelPR:
name: Label PR for update-versions
name: Set merge-and-release-extension-compatible label
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the name since I think the new name would be more intuitive for a random engineer. Not deeply attached to this change.

@angelowilliams
Copy link
Contributor Author

@MikeRawding -- I requested review from you on this. No rush at all. I can also look for review elsewhere if you're swamped with other work, just let me know.

@MikeRawding
Copy link
Contributor

The extension actually has a "backup" check which looks for the 'Update versions' action the list of actions at the bottom of the page. So, the extension will still show up with this change.

The offending line: https://github.com/nicheinc/merge-and-release-extension/blob/master/src/index.tsx#L19

History on why this was added: https://github.com/nicheinc/merge-and-release-extension/issues/24

Perhaps we can solve that issue in another way? I'm all for doing more in GHA and less HTML parsing.

@angelowilliams
Copy link
Contributor Author

angelowilliams commented Jan 16, 2025

The extension actually has a "backup" check which looks for the 'Update versions' action the list of actions at the bottom of the page. So, the extension will still show up with this change.

The offending line: https://github.com/nicheinc/merge-and-release-extension/blob/master/src/index.tsx#L19

History on why this was added: nicheinc/merge-and-release-extension#24

Perhaps we can solve that issue in another way? I'm all for doing more in GHA and less HTML parsing.

After thinking about this a little more, I actually think we're fine to remove that Update versions check. With this PR changing the behavior such that the labelPR job runs for both opened and synchronize events, in that "you open a PR and then push a commit right after" edge case, the PR will still end up with the label.

I tested this out and confirmed that this is the case. First, I added that same concurrency check to my actions-go-ci test branch. Then, I created a gha-test-environment PR, which triggered this run, which got cancelled by a very fast commit triggering this second run, ultimately resulting in the MnR label being placed upon our PR. We'd expect to see the same exact behavior in the website repo.

I opened a PR for the extension itself here: https://github.com/nicheinc/merge-and-release-extension/pull/34

EDIT: I also removed this spooky warning about the Update versions job name since that is no longer relevant with these changes.

Copy link
Contributor

@MikeRawding MikeRawding left a comment

Choose a reason for hiding this comment

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

Love this, cleans up 2 things at once and removed a selector from the extension!

@angelowilliams angelowilliams merged commit 8daf581 into main Jan 29, 2025
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