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

Enable fetch plan for current app and account #1630

Merged

Conversation

blacelle
Copy link
Contributor

Description

Fixes #1613

This enables accessing the plan with current GithubApp on current GHInstallation.

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".

pom.xml Outdated
@@ -600,6 +600,12 @@
<version>2.0.3</version>
<scope>test</scope>
</dependency>
<dependency>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used to facilitate loading a custom private JWK for a GHApp

Copy link
Member

Choose a reason for hiding this comment

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

Is there an other option aside from adding a new test dependency? How much work does this save?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered relying on jjwt, but the feature does not exist yet into there, and people points back to nimbus-jose: jwtk/jjwt#236 (comment)

I would have to go through the JWK to PEM procedure, which is a burden given my own setup (i.e. my app using this lib relies on a JWK, not a PEM). Also, it would be more error-prone to rely on a given PEM content through environment variable (which is again my setup, and how I suggest to manage custom privateKey here).

I tried copying some classes from Nimbus to fill the gap, but it seems to require quite a bunch of classes. I'm having another try but focusing on the minimal set of methods to get a PrivateKey.

My only justification for this additional lib is existing tests for GitHub App APIs has not dynamic mechanisms for now (neither from local file, or from Env). I may then just drop this code, and be potentially discouraged to contribute here (this is a weak argument as I would be happy to fit into any existing mechanism, the current option would be to copy my own privateKey as a test resources, with a risk of pushing it publicly).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm dropping nimbus, with a mechanism to load a PEM from a Path provided as environment variable (similarly to the pushed code). However, I will not be a user of this mechanism. It will pave the way for next contribution requiring a custom JWK.

@bitwiseman
Copy link
Member

It looks like you're still working on this. Setting to draft for now.

@bitwiseman bitwiseman marked this pull request as draft March 20, 2023 18:53
@bitwiseman bitwiseman self-requested a review March 20, 2023 18:53
GHApp app = gitHub.getApp();

GHAppInstallation appInstallation;
if (Set.of(TEST_APP_ID_1, TEST_APP_ID_2, TEST_APP_ID_3).contains(Long.toString(app.getId()))) {
Copy link
Contributor Author

@blacelle blacelle Mar 20, 2023

Choose a reason for hiding this comment

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

This switch leads to invalid recordings: I've done it to prevent recording the N installations of my real-world app. However, when re-running the tests without my custom JWK, the test will list all installations, which will match no stub (as we recorded the other branch of the if, which was focusing on a dedicated repo.)

This remain useful, however one has to edit the recordings manually (e.g. copy-pasting the installations listing from another test).

@bitwiseman Would you mind granting me access to the test repo/app ? It may be useful on related matters not requiring markplace-listing.

@codecov
Copy link

codecov bot commented Mar 20, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (afe6732) 79.86% compared to head (e09bd39) 79.88%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1630      +/-   ##
============================================
+ Coverage     79.86%   79.88%   +0.02%     
- Complexity     2191     2195       +4     
============================================
  Files           208      209       +1     
  Lines          6659     6667       +8     
  Branches        364      364              
============================================
+ Hits           5318     5326       +8     
  Misses         1127     1127              
  Partials        214      214              
Impacted Files Coverage Δ
...ain/java/org/kohsuke/github/GHAppInstallation.java 92.10% <100.00%> (+0.21%) ⬆️
.../java/org/kohsuke/github/GHMarketplaceAccount.java 87.50% <100.00%> (+1.78%) ⬆️
...uke/github/GHMarketplacePlanForAccountBuilder.java 100.00% <100.00%> (ø)
.../github/extras/authorization/JWTTokenProvider.java 89.18% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@blacelle blacelle marked this pull request as ready for review March 20, 2023 20:49
@blacelle
Copy link
Contributor Author

(As a side-note, the Wiremock tooling to stub api.github.com is very inspiring. I'm duplicating the related code in my own project.)

@blacelle
Copy link
Contributor Author

@bitwiseman This is ready for review/merging.

Copy link
Member

@bitwiseman bitwiseman left a comment

Choose a reason for hiding this comment

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

Missing coverage for one method. Fix that and we can merge.

.java-version Outdated Show resolved Hide resolved
@bitwiseman bitwiseman self-requested a review April 15, 2023 22:51
@bitwiseman bitwiseman merged commit 77f7f81 into hub4j:main Apr 15, 2023
@bitwiseman
Copy link
Member

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.

Feature Request - GET /marketplace_listing/accounts/{account_id}
2 participants