[JENKINS-36240] Option to look up trust status directly from GitHub API #148
Conversation
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
| */ | ||
| @DataBoundConstructor | ||
| public TrustPermission(@NonNull String permission) { | ||
| GHPermissionType permissionType = GHPermissionType.ADMIN; |
There was a problem hiding this comment.
Why is this even configurable? The only useful value is WRITE. If you can push to the repository, you could change the Jenkinsfile, thus you are trusted.
There was a problem hiding this comment.
Why? I claim it is not, and we are making users make a decision here, so you need to back up why this would make sense (and in fact be the default value!).
There was a problem hiding this comment.
I tend to agree with @jglick. For these purposes, WRITE is just as dangerous as ADMIN, so why do we care about ADMIN?
| * | ||
| * @since TODO | ||
| */ | ||
| public abstract class GitHubPermissionsSource { |
There was a problem hiding this comment.
Why does this need to be public? Seems like an implementation detail.
There was a problem hiding this comment.
If you want to instantiate the request in your own SCMSource and you use the trait you need to be able to provide one of these
There was a problem hiding this comment.
What SCMSource would that be? Best to not expose a public API without a concrete use case.
| @CheckForNull | ||
| private GHRepository repository; | ||
| /** | ||
| * The resolved permissions keyed by user. |
There was a problem hiding this comment.
For the duration of the request to scm api
|
Desirable to have hub4j/github-api#358 but probably unnecessary. |
|
there are some findbugs problems with this - but looks close. This is a desired feature, a subtle one but I have seen people rage over it when they realise what the problem is with trust. |
See JENKINS-36240
Replaces #96
To Do
@reviewbybees