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 MAINTAIN and TRIAGE permissions #1672

Merged
merged 3 commits into from
Jun 5, 2023

Conversation

MarkEWaite
Copy link
Contributor

Add MAINTAIN and TRIAGE permissions to permission type

I've not included links to the GitHub REST API documentation describing those pemrissions because I've been unable to find them listed in the REST API documentation. I'm confident that they are in the documentation, but my search and navigation skills are insufficient to find them.

Failures for MAINTAIN permission on ci.jenkins.io are documented in:

Failures for TRIAGE permission on other systems are documented in:

  • Community post - Multibranch pipline fails because TRIAGE enum doesn't exist

Special thanks to @PierreBietz for his investigation work and the test case that shows the issue.

Requesting review from @jglick and @basil in case I've missed something important in the implementation.

Before submitting a PR:

  • Changes must not break binary backwards compatibility. If you are unclear on how to make the change you think is needed while maintaining backward compatibility, CONTRIBUTING.md for details.
  • Add JavaDocs and other comments explaining the behavior.
  • When adding or updating methods that fetch entities, add @link JavaDoc entries to the relevant documentation on https://docs.github.com/en/rest .
  • Add tests that cover any added or changed code. This generally requires capturing snapshot test data. See CONTRIBUTING.md for details.
  • Run mvn -D enable-ci clean install site locally. If this command doesn't succeed, your change will not pass CI.
  • Push your changes to a branch other than main. You will create your PR from that branch.

When creating a PR:

  • Fill in the "Description" above with clear summary of the changes. This includes:
    • If this PR fixes one or more issues, include "Fixes #" lines for each issue.
    • Provide links to relevant documentation on https://docs.github.com/en/rest where possible. If not including links, explain why not.
  • All lines of new code should be covered by tests as reported by code coverage. Any lines that are not covered must have PR comments explaining why they cannot be covered. For example, "Reaching this particular exception is hard and is not a particular common scenario."
  • Enable "Allow edits from maintainers".

PierreBtz and others added 2 commits June 5, 2023 10:58
Fixes hub4j#1671

https://community.jenkins.io/t/multibranch-pipline-fails-because-triage-enum-doesnt-exist/7800
reports that the TRIAGE permission is unknown to the GitHub api library.

jenkins-infra/helpdesk#3617 reports that the
MAINTAINER permission is unknown to the GitHub api library.

This adds both the triage and the maintain permission to the
enumeration so that new releases of the plugins depending on
this library can be done to avoid the stack trace reported in
jenkins-infra/helpdesk#3617

That stack trace includes:

java.lang.IllegalArgumentException: No enum constant org.kohsuke.github.GHPermissionType.MAINTAIN

Special thanks to @pierrebeitz for the test case with wiremock.
@codecov
Copy link

codecov bot commented Jun 5, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (7403e25) 79.93% compared to head (49a93bf) 79.93%.

❗ Current head 49a93bf differs from pull request most recent head ff16417. Consider uploading reports for the commit ff16417 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #1672   +/-   ##
=========================================
  Coverage     79.93%   79.93%           
  Complexity     2208     2208           
=========================================
  Files           212      212           
  Lines          6687     6689    +2     
  Branches        364      364           
=========================================
+ Hits           5345     5347    +2     
  Misses         1128     1128           
  Partials        214      214           
Impacted Files Coverage Δ
...main/java/org/kohsuke/github/GHPermissionType.java 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@jglick jglick left a comment

Choose a reason for hiding this comment

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

As in #1671 (comment) I would suggest adding an UNKNOWN or OTHER constant and making GHPermission.getPermissionType fall back to that.

I do not believe any change to github-branch-source is required since https://github.com/jenkinsci/github-branch-source-plugin/blob/d391eef681ae51743402ef08deac2b94565b09aa/src/main/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait.java#L319-L325 only cares about ADMIN and WRITE. You should of course push a release of https://plugins.jenkins.io/github-api/.

When GitHub adds a new permission, return UNKNOWN with less permission
than NONE.
@car-roll car-roll merged commit 4cbae90 into hub4j:main Jun 5, 2023
@MarkEWaite MarkEWaite deleted the pbeitz/maintain-permission branch June 5, 2023 20:59
@car-roll car-roll added the bug label Jun 5, 2023
@car-roll
Copy link
Collaborator

car-roll commented Jun 5, 2023

@bitwiseman Looks like I don't have release permissions. would you be able to cut a release for this issue? many thanks!

@jglick
Copy link
Contributor

jglick commented Jun 5, 2023

Better hold off until we have analyzed the impact of the GitHub.com rollback.

@MarkEWaite
Copy link
Contributor Author

Better hold off until we have analyzed the impact of the GitHub.com rollback.

As far as I can tell, the new entries in the enumeration do not harm the plugin even after the GitHub REST API rollback. The rollback has happened and scan repository is working as expected with the patched github API library.

GitHub announced maintain and triage in a 2019 blog post, but uses the word roles to describe them rather than calling them permissions. I'm not sure if that means that maintain and triage do not belong in the GHPermissionType enumeration or if they belong there.

I'm OK if we roll back this change and hope that GitHub won't make this type of change in the future, or we could partially revert it so that we don't add maintain or triage to the enumeration but would not throw an exception if values were added to the enumeration

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

Successfully merging this pull request may close these issues.

GHRepository#getPermission fails with a user having the maintain role
4 participants