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

Generify OrgAppInstallationAuthorizationProvider to support any kind of app lookup. #1624

Merged
merged 7 commits into from
May 23, 2023

Conversation

PierreBtz
Copy link
Contributor

@PierreBtz PierreBtz commented Mar 2, 2023

Description

The OrgAppInstallationAuthorizationProvider only works with organization level apps, leaving out user apps. The bulk of the class logic remains useful to perform application lookup other ways (eg using the installation id), or even to lookup for user application.

This PR provides a way for the API user to provide its own lookup logic.

This PR is submitted as a draft because:

  • I would need access to the hub4j-test-org as well as the installation id of the test app to finish writing the tests.
  • I'm not 100% convinced OrgAppInstallationAuthorizationProvider should be deprecated. The way I see it, it's not useless and just a maintenance burden that you might want to remove in the future after a deprecation phase :)

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 as appropriate. Consider including links in comments to 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.
  • 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".

alecharp added a commit to alecharp/plugin-health-scoring that referenced this pull request Mar 8, 2023
@bitwiseman
Copy link
Member

@PierreBtz I've invited you to the hub4j-test-org.

@PierreBtz PierreBtz marked this pull request as ready for review April 14, 2023 09:44
@PierreBtz
Copy link
Contributor Author

@bitwiseman thanks for the access. I finally had time to complete this work...

@bitwiseman bitwiseman self-requested a review April 15, 2023 22:49
@codecov
Copy link

codecov bot commented Apr 15, 2023

Codecov Report

Patch coverage: 93.33% and no project coverage change.

Comparison is base (5380f59) 79.89% compared to head (ba0f0a0) 79.89%.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #1624   +/-   ##
=========================================
  Coverage     79.89%   79.89%           
  Complexity     2197     2197           
=========================================
  Files           209      210    +1     
  Lines          6670     6671    +1     
  Branches        364      364           
=========================================
+ Hits           5329     5330    +1     
  Misses         1127     1127           
  Partials        214      214           
Impacted Files Coverage Δ
...rization/AppInstallationAuthorizationProvider.java 92.85% <92.85%> (ø)
...ation/OrgAppInstallationAuthorizationProvider.java 100.00% <100.00%> (+6.66%) ⬆️

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

@PierreBtz
Copy link
Contributor Author

@bitwiseman gentle ping, is there anything blocking this PR from being merged? Thanks for your time.

@bitwiseman bitwiseman merged commit 75a925e into hub4j:main May 23, 2023
@PierreBtz
Copy link
Contributor Author

@bitwiseman thanks for taking care of the merge, would you mind cutting a release ?

@bitwiseman
Copy link
Member

@PierreBtz
I'll get in the next day.

@bitwiseman
Copy link
Member

@PierreBtz
Copy link
Contributor Author

Thanks!!

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.

2 participants