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 pullRequestAuthorRole #715

Merged

Conversation

abelsiqueira
Copy link
Contributor

Let me know how I can improve it.

Closes #707

Copy link
Owner

@bobvanderlinden bobvanderlinden 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 already looking quite good! Some suggestions and I think this also needs an addition in the readme.

src/conditions/pullRequestAuthorRole.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
src/conditions/requiredAuthorRole.ts Outdated Show resolved Hide resolved
src/conditions/pullRequestAuthorRole.ts Outdated Show resolved Hide resolved
Copy link
Owner

@bobvanderlinden bobvanderlinden left a comment

Choose a reason for hiding this comment

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

The type of requiredAuthorRole in config.ts can be CommentAuthorAssociation. In addition, the readme needs a section for the new option.

Copy link
Owner

@bobvanderlinden bobvanderlinden left a comment

Choose a reason for hiding this comment

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

Looks good to me now 👍 Let me know whether you have any changes in mind. If not, I'll merge and deploy this to production.

@abelsiqueira
Copy link
Contributor Author

Thanks for the update. From what I understood, we'll write something like

rules:
  - pullRequestOwnerRole: CommentAuthorAssociation.OWNER
    minApprovals:
      OWNER: 1
  - minApprovals:
      OWNER: 2

is that correct?

@bobvanderlinden
Copy link
Owner

Yes, that should be possible. Best would be to create an integration test for your explicit case if you want to be sure this will work now and in the future.

Example of a similar integration test can be found here:

it('to merge when one rule and the global configuration passes', async () => {
const config = `
rules:
- minApprovals:
OWNER: 1
- requiredLabels:
- merge
`
const pullRequestInfo = createPullRequestInfo({
reviews: {
nodes: [
approvedReview({
authorAssociation: CommentAuthorAssociation.CONTRIBUTOR
})
]
},
commits: createCommitsWithCheckSuiteWithCheckRun({
checkRun: successCheckRun
}),
labels: {
nodes: [
{ name: 'merge' }
]
}
})
const github = createGithubApiFromPullRequestInfo({
pullRequestInfo,
config
})
const app = createApplication({
appFn,
logger: createEmptyLogger(),
github
})
await app.receive(
createPullRequestOpenedEvent({
owner: 'bobvanderlinden',
repo: 'probot-auto-merge',
number: 1
})
)
expect(github.pulls.merge).toHaveBeenCalled()
})

@abelsiqueira
Copy link
Contributor Author

Sorry for the very long delay. I've created a test and hopefully it will work now.

Copy link
Owner

@bobvanderlinden bobvanderlinden left a comment

Choose a reason for hiding this comment

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

Thanks for the work! This looks really useful and the implementation well-tested. Only thing missing is an addition to the README. Other than that, this looks good to merge 👍

@abelsiqueira
Copy link
Contributor Author

Thanks, I'm glad it'll be useful. I have created a new commit with the README addition.

@probot-auto-merge probot-auto-merge bot merged commit ad56978 into bobvanderlinden:master Aug 24, 2020
@github-actions
Copy link

🎉 This PR is included in version 1.3.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Number of approvals depending on PR author
2 participants