-
-
Notifications
You must be signed in to change notification settings - Fork 16
fix: Accept mixed-case owner names #937
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import { Probot, Context } from 'probot' | ||
| import { PullRequestEvent, PullRequestReviewEvent } from '@octokit/webhooks-types' | ||
| import { Context, Probot } from 'probot' | ||
|
|
||
| module.exports = (app: Probot) => { | ||
| app.on(['pull_request.opened', 'pull_request.reopened', 'pull_request.labeled', 'pull_request.edited', 'pull_request_review'], async (context) => { | ||
|
|
@@ -39,8 +39,9 @@ module.exports = (app: Probot) => { | |
| } | ||
| } | ||
|
|
||
| // reading pull request owner info and check it with configuration | ||
| const ownerSatisfied = config.from_owner.length === 0 || config.from_owner.includes(pr.user.login) | ||
| // reading pull request owner info and check it with configuration (case insensitive) | ||
| const lowerLogin = pr.user.login.toLowerCase() | ||
| const ownerSatisfied = config.from_owner.length === 0 || config.from_owner.some((owner: string) => owner.toLowerCase() === lowerLogin) | ||
|
Comment on lines
+43
to
+44
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm trying to address the situation where the |
||
|
|
||
| // reading pull request labels and check them with configuration | ||
| let requiredLabelsSatisfied | ||
|
|
@@ -75,7 +76,7 @@ module.exports = (app: Probot) => { | |
| } | ||
| } else { | ||
| // one of the checks failed | ||
| context.log('Condition failed! \n - missing required labels: %s\n - PR owner found: %s', requiredLabelsSatisfied, ownerSatisfied) | ||
| context.log('Condition failed! \n - has required labels: %s\n - PR owner found: %s', requiredLabelsSatisfied, ownerSatisfied) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
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_labelsfield, and succeeds if it does [with an empty array]).