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

Improved how it checks for github collaborator access #111

Merged

Conversation

xocasdashdash
Copy link

CollaboratorService collaboratorService = new CollaboratorService(client);

try {
return collaboratorService.isCollaborator(repository, User);
Copy link
Author

Choose a reason for hiding this comment

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

This ends up calling this endpoint:

/repos/${OWNER}/${REPO}/collaborators/${username}

So it leaves all of the decision logic back on github instead of here.

return false;
}
}

public static List<String> getCollaborators(@Nonnull final Job<?,?> job) {
Copy link
Author

Choose a reason for hiding this comment

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

I think this method can be deleted as its not used anymore. I can do that with no issue

Copy link
Member

Choose a reason for hiding this comment

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

yeah I don't see an issue with removing it.

@xocasdashdash
Copy link
Author

Sorry to ping you @aaronjwhiteside , is there any way of speeding up getting this PR merged and a new release cut? not sure who to ping about this and it's been 2+ months since i opened it.

@aaronwalker
Copy link
Member

@xocasdashdash sorry it's been a crazy couple of months. I will review this PR and let you know. I think the changes look pretty good though

Signed-off-by: Joaquin Fernández Campo <joaquin.campo@paddle.com>
@xocasdashdash
Copy link
Author

@aaronwalker Updated the PR removing the dead code, let me know if there's anything else to do!

@xocasdashdash
Copy link
Author

@aaronwalker hey, sorry to ping, do you think this can get merged now?

@aaronwalker
Copy link
Member

@aaronwalker hey, sorry to ping, do you think this can get merged now?

Just a quick one before I merge this... Have you tested the Incrementals plugin version https://repo.jenkins-ci.org/incrementals/org/jenkins-ci/plugins/pipeline-github/2.8-140.9440018594b6/ ?

@xocasdashdash
Copy link
Author

I did not know this existed! Will get back to you once i've validated it!

@TueDissingWork
Copy link

TueDissingWork commented Jan 6, 2023

UPDATE: works fine with this version: 2.8-140.9440018594b6

Was simple missing correct auth config for the repo on the GitHub app.
When using version 2.8-138.d766e30bb08b is also works. Nice!

Hey, just tried the mentioned version @aaronwalker, but to avail.
Still getting: Comment Author: USERNAME is not a collaborator, and is therefore not authorized to trigger a build., when trying to trigger the build with a comment. I am, as well, part of a team.

Job: devops/autochange-pr-flow/PR-1,
IssueComment: GHIssueComment@172b2357[body=test this please,url=https://githuburl/api/v3/repos/org/repo-name/issues/comments/.....],
Comment Author: USERNAME is not a collaborator, and is therefore not authorized to trigger a build.

@xocasdashdash
Copy link
Author

Nice!
I haven't been able to test it myself but if @TueDissingWork has, let's get this merged! :)

@xocasdashdash
Copy link
Author

@aaronwalker are you ok with @TueDissingWork tests?

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