-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
fix: Accept mixed-case owner names #937
base: master
Are you sure you want to change the base?
Conversation
This PR: - Accepts mixed-case owner names - Adds a test to that effect - Updates the README to clarify the instructions and correctly state that `blacklisted_labels` is required. - Changes the error message to read more understandably (i.e. the original text had inverted logic in its text).
@dkhmelenko : hi there! Thanks for this super-helpful tool. Would you mind reviewing this? I had run into this problem in my own use of the tool. |
@@ -98,7 +102,7 @@ If the value set to `one_of`, then it's enough to have only of the `required_lab | |||
--- | |||
|
|||
### blacklisted_labels | |||
|
|||
**mandatory** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you make blacklisted_labels as a mandatory property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dkhmelenko : please correct me if I'm wrong, but I ran into #753 (i.e. my GitHub Action fails if my autoapprove.yml doesn't have the blacklisted_labels
field, and succeeds if it does [with an empty array]).
const lowerLogin = pr.user.login.toLowerCase() | ||
const ownerSatisfied = config.from_owner.length === 0 || config.from_owner.some((owner: string) => owner.toLowerCase() === lowerLogin) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an interesting case, out of curiosity: when can it happen that we have different cases of the owner? Or do you want to prevent a situation when GitHub username has different case than the one in the config file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to address the situation where the yml
file contains a different case from what GitHub has. Since GitHub handles are case-insensitive, it feels like the yml
format should similarly not enforce casing.
This PR:
blacklisted_labels
is required.