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

fix: Accept mixed-case owner names #937

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fivecar
Copy link

@fivecar fivecar commented Feb 14, 2025

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).

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).
@fivecar
Copy link
Author

fivecar commented Feb 14, 2025

@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**
Copy link
Owner

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?

Copy link
Author

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]).

Comment on lines +43 to +44
const lowerLogin = pr.user.login.toLowerCase()
const ownerSatisfied = config.from_owner.length === 0 || config.from_owner.some((owner: string) => owner.toLowerCase() === lowerLogin)
Copy link
Owner

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?

Copy link
Author

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.

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