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 label support to REVIEWERS and NOTIFIED files. #89

Merged
merged 3 commits into from
Nov 12, 2024
Merged

Conversation

csilvers
Copy link
Member

@csilvers csilvers commented Nov 12, 2024

Summary:

This lets you specify rules like this:

something_changed: **/* @notifyme

And when we add the NOTIFIED section to the github PR, it will now say:

@notifyme (something_changed): file1 file2 file3

(For rules without labels, the output is unchanged.)

This is helpful for knowing why you were being notified for a
particular PR, since as we add more NOTIFIED rules it can get harder
to tell.

I added the support for REVIEWERS as well, since it was easy, though
I'm not sure how useful it is there.

While in the area, I couldn't help but clean up the existing code a
bit, especially simplifying some of the regexp-parsing logic (at the
cost of more parentheses in the regexp).

Issue: https://khanacademy.atlassian.net/browse/FEI-5970

Test plan:

yarn flow
yarn jest

This lets you specify rules like this:
```
allfiles: **/* @notifyme
```

And when we add the NOTIFIED section to the github PR, it will now say:
```
@notifyme (allfiles): file1 file2 file3
```
(For rules without labels, the output is unchanged.)

This is helpful for knowing _why_ you were being notified for a
particular PR, since as we add more NOTIFIED rules it can get harder
to tell.

I added the support for REVIEWERS as well, since it was easy, though
I'm not sure how useful it is there.

While in the area, I couldn't help but clean up the existing code a
bit, especially simplifying some of the regexp-parsing logic (at the
cost of more parentheses in the regexp).

Issue: https://khanacademy.atlassian.net/browse/FEI-5970

Test plan:
yarn flow
yarn jest
@csilvers csilvers self-assigned this Nov 12, 2024
@khan-actions-bot
Copy link
Contributor

khan-actions-bot commented Nov 12, 2024

Gerald

Required Reviewers
  • @Khan/github-actions for changes to .github/NOTIFIED, .github/REVIEWERS, dist/index.js, dist/source-map-support.js, setup-files/Gerald-README.md, setup-files/gerald-pr.yml, src/constants.js, src/utils.js, src/__test__/utils.test.js

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

@khan-actions-bot khan-actions-bot requested a review from a team November 12, 2024 18:41
Copy link
Member

@lillialexis lillialexis left a comment

Choose a reason for hiding this comment

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

Thanks so much, and thanks for the clean-up!

My only feedback is to please add more examples, because the allfiles example, in isolation, if I didn't know what I was looking at, my brain would think that it had to do with all the files specifically, as opposed to an example of how a label would work. If that makes sense. 😄

@lillialexis
Copy link
Member

Also, does this only work with the file-matching part of Gerald or the regex matching too? If the latter, a regex of a regex, are there any cases where we would get false positives because of a colon? I think not, because probably it's looking for the regex to be surrounded by /s... 🤔

@csilvers
Copy link
Member Author

I've renamed allfiles to something_changed. Does that make things clearer?

@csilvers
Copy link
Member Author

(In the PR description. In the PR itself I have:

# This rule will notify @owner1 on changes to all files, and the rule
# has a label "allfiles", which will be included in the github PR info.
# allfiles: **/*                @owner1

which seems pretty clear? But let me know if not.

@csilvers
Copy link
Member Author

Also, does this only work with the file-matching part of Gerald or the regex matching too? If the latter, a regex of a regex, are there any cases where we would get false positives because of a colon? I think not, because probably it's looking for the regex to be surrounded by /s... 🤔

I mention this in the commit message. Regexps have to start with a ", so there's no chance of confusion.

(And definitely it works with both!)

@csilvers csilvers merged commit 4f836bc into main Nov 12, 2024
4 checks passed
@csilvers csilvers deleted the add-labels branch November 12, 2024 19:39
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