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.

Sign up for GitHub

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

Jump to bottom

Implement GitHub App API methods #522

Merged
merged 16 commits into from
Oct 4, 2019
Merged

Implement GitHub App API methods #522

merged 16 commits into from
Oct 4, 2019

Conversation

PauloMigAlmeida
Copy link
Contributor

This PR implements the GitHub Apps API workflow. Things to be noted:

  • I assumed that JWT is encoded externally and its JWT token is passed as a parameter to be used as an authentication mechanism (like it's done with the OAuth token currently). Implementing JWT encoding logic seemed too cumbersome for the project's value proposition (IMHO)

  • Methods that can only be requested via JWT as part of the Github App workflow are documented

  • On the Requester class, I had to make the with(String key, Collection<String> value) more generic as just adding another method like with(String key, Collection<Integer> value) wouldn't do the trick...and adding a specific method for this seemed weird...but let me know if you want to have this implemented in another way or if you're fine with the approach taken.

If you find that this contribution is a good addition to the project, please accept this PR.

Best regards,

@ghost
Copy link

ghost commented Jun 11, 2019

DeepCode analyzed this pull request.
There is 1 new info report.

Click to see more details.

@PauloMigAlmeida
Copy link
Contributor Author

Added @Preview and @deprecated annotations to methods in which the API is still in preview

@PauloMigAlmeida
Copy link
Contributor Author

ping @kohsuke @bitwiseman

Hi guys, is there anything I can do to make this PR mergeable?

@webratz
Copy link

webratz commented Jul 25, 2019

would really ❤️ to see that merged, as its a pre-requisite to make Jenkins a GitHub App, which would help a lot, especially for bigger setups, see also https://issues.jenkins-ci.org/browse/JENKINS-57351

@bitwiseman
Copy link
Member

@PauloMigAlmeida
My apologies for the delay. I've been buried under other work and I will be for at least another couple weeks, but make the time to be responsive to this discussion.

This looks reasonable to me from cursory review. The Requester change isn't an API break, right? If so, that also seem fine.

I'm new to this project and very uncomfortable with how little testing it includes, but I also understand how testing is difficult in this area.

Can you see any way to add more tests for these changes API? I would be far more comfortable if there were.

@PauloMigAlmeida
Copy link
Contributor Author

PauloMigAlmeida commented Aug 7, 2019 •
edited
Loading

@bitwiseman
No problem at all and thanks for taking the time to take a look at this PR

This looks reasonable to me from cursory review. The Requester change isn't an API break, right? If so, that also seems fine.

You're correct. This doesn't break backwards API compatibility.

Can you see any way to add more tests for these changes API? I would be far more comfortable if there were.

Absolutely! I think I can develop something using MockServer to mimic Github server responses and ensure that those new methods work as expected. Let me know if you have another strategy for testing it that you want me to follow.

I will ping you once I've finished it (probably around tomorrow)

@bitwiseman
Copy link
Member

bitwiseman commented Aug 8, 2019 •
edited
Loading

@PauloMigAlmeida
The only thing we have in this area is #316 and #382.

One of the clients of this library, the Jenkins github-branch-source-plugin has a number of WireMock-based tests, but I would be happy to see any reasonable set of automated tests.

@PauloMigAlmeida
Add tests for GithubApp integration;
4703f2d 
Add wiremock-standalone library;

Signed-off-by: Paulo Almeida <[email protected]>
@PauloMigAlmeida
Copy link
Contributor Author

@bitwiseman
I read the threads you mentioned and in both cases, it seems to boil down to the reference implementation on the github-branch-source-plugin.

Having said that, I implemented the tests using canned HTTP responses (WireMock-based tests). Let me know if you want me to change anything else before merging it.

@PauloMigAlmeida
Copy link
Contributor Author

ping @bitwiseman

@PauloMigAlmeida
Copy link
Contributor Author

ping @bitwiseman

Let me know if there is anything you want me to change in order to get this approved.

PS.: I saw that given the last few merges you did on the master branch caused this PR to conflict, so I just merged with the most up-to-date commit.

@bitwiseman
Copy link
Member

@PauloMigAlmeida
I haven't had time to do a deep review, but generally this looks reasonable.

I have made a mocking framework that should simplify your tests some - each test method gets it's only directory which should mean you don't have to read each payload. Could you move this to use that structure and then I can do a final review?

Why do most of the methods have @Preview @Deprecated on them?

bitwiseman
bitwiseman requested changes Sep 10, 2019
View reviewed changes
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.

Update based on new test framework needed.

@PauloMigAlmeida
Copy link
Contributor Author

Hi @bitwiseman, all good.

Sure, I can definitely update my tests to follow the new test framework. I will probably do it tomorrow. Stay tuned 😊

In regards to the preview/deprecated annotations, that’s a solution proposed by kohsuke for dealing with github preview apis.

It is documented here: https://github.com/github-api/github-api/blob/master/src/main/java/org/kohsuke/github/Preview.java

If that doesn’t apply anymore, let me know and I will gladly remove them

@bitwiseman
Copy link
Member

@PauloMigAlmeida
Re "Preview" - Ah, got it. Yes, that still applies please continue doing that.

@PauloMigAlmeida
Copy link
Contributor Author

@bitwiseman
I updated my tests to make use of the new wiremock structure you created.

Let me know if you need anything to be amended before you can merge it into master.

@PauloMigAlmeida
Copy link
Contributor Author

ping @bitwiseman

@PauloMigAlmeida
Merge branch 'master' of https://github.com/kohsuke/github-api
a163c56 
� Conflicts:
�	src/main/java/org/kohsuke/github/GitHubBuilder.java
�	src/test/java/org/kohsuke/github/GitHubTest.java
@PauloMigAlmeida
Merge from upstream;
0f7a5f1 
Remove servlet exclusion due to previous JDK version;

Signed-off-by: PauloMigAlmeida <[email protected]>
@PauloMigAlmeida
Copy link
Contributor Author

PauloMigAlmeida commented Oct 3, 2019 •
edited
Loading

@bitwiseman

I saw that the last few merges you did on the master branch caused this PR to conflict, so I just merged with the most up-to-date commit.

image

I would love to hear from you if there is anything you want me to change so that we can get this merged 😄

@PauloMigAlmeida
Copy link
Contributor Author

@bitwiseman the build broke due to a legacy code which isn't connected to this PR.

image

bitwiseman
bitwiseman approved these changes Oct 4, 2019
View reviewed changes
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.

This looks good to me.

@bitwiseman
Copy link
Member

Fixed the failures in master, so this is passing now.

@bitwiseman bitwiseman merged commit 8da6db9 into hub4j:master Oct 4, 2019
@sco11morgan
Copy link

🎆 🎉

@PauloMigAlmeida
Copy link
Contributor Author

🤩🤩🤩🤩

@bitwiseman bitwiseman mentioned this pull request Oct 11, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bitwiseman bitwiseman bitwiseman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@PauloMigAlmeida @webratz @bitwiseman @sco11morgan