Skip to content

Conversation

Goldspear
Copy link
Contributor

Fixes #88098

What Changed:

  1. Check_labels CI job no longer fails, but only adding a comment prompt if the required labels is not satisfied.
  2. Merge would fail with a comment instead if the required labels are not satisfied.

CI check_labels won't fail but leaving a comment, fail merge attempt instead.
@Goldspear Goldspear requested a review from a team as a code owner January 15, 2023 18:38
@pytorch-bot
Copy link

pytorch-bot bot commented Jan 15, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/92225

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 8b4d31e:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 15, 2023

CLA Missing ID CLA Not Signed

@pytorch-bot pytorch-bot bot added the topic: not user facing topic category label Jan 15, 2023
Copy link
Contributor

@ZainRizvi ZainRizvi left a comment

Choose a reason for hiding this comment

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

This is looking really good! It just needs a couple more things:

  1. Could you please add unit tests in .github/scripts/test_trymerge.py
  2. We'll also want to remove the check_labels.py and move the corresponding unit tests from .github/workflows/check-labels.yml to test_trymerge.py
  3. Remove the .github/workflows/check-labels.yml workflow

Your call on whether you'd like to do #2 & #3 as part of this PR or as a separate PR

@Goldspear
Copy link
Contributor Author

Thanks @ZainRizvi for the prompt review and suggestions!

The original issue description suggested to keep the check_label CI job, hence the original PR, but I agree it's redundant and should be cleaned up. The new commit now has it removed and the relevant unit tests refractored accordingly. Please kindly take another look when you have a chance :)

@Goldspear Goldspear requested a review from ZainRizvi January 17, 2023 15:45
@ZainRizvi
Copy link
Contributor

ZainRizvi commented Jan 17, 2023

Thanks @ZainRizvi for the prompt review and suggestions!

The original issue description suggested to keep the check_label CI job, hence the original PR, but I agree it's redundant and should be cleaned up. The new commit now has it removed and the relevant unit tests refractored accordingly. Please kindly take another look when you have a chance :)

Actually, thinking it over again I'd prefer the approach described in the ticket where the label message is still shared with devs earlier, but it doesn't turn into a red X failing the CI.

Would you mind putting it back in that format? Apologies for the churn

@Goldspear
Copy link
Contributor Author

Actually, thinking it over again I'd prefer the approach described in the ticket where the label message is still shared with devs earlier, but it doesn't turn into a red X failing the CI.

Ah that makes sense, sure thing, I will add the check_label ci job back (in the new PR instead) for comment only. Most of the tests/moved fcns can stay where they are I think, so not much of a waste :)

@Goldspear Goldspear closed this Jan 18, 2023
pytorchmergebot pushed a commit that referenced this pull request Jan 19, 2023
Fixes #88098

### What Changed
* Moved `check_label.py` logic into `trymerge.py`
* Refactored relevant unittests
* ~~Dropped~~ Refactored `check_label.py` ci job

### Tests
`python .github/scripts/test_trymerge.py`
`python .github/scripts/test_check_labels.py`
`make lint & lintrunner -a`

### Notes to reviewers
This PR replaces the [original PR](#92225) to workaround the sticky EasyCLA failure mark on its first commit.

Pull Request resolved: #92309
Approved by: https://github.com/ZainRizvi
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Label Check] Move label check logic from CI to mergebot

2 participants