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

Additional GitHub Apps APIs #1854

Merged

ryangribble merged 31 commits into master from installations-for-user

Sep 3, 2018

Contributor

StanleyGoldman commented Aug 7, 2018 •

edited

Loading

This API call comes in handy for integrators of GitHub Apps:


          Adding functionality to query installations for user

a80e226

StanleyGoldman changed the title ~~Adding GitHub Apps: List installations for user~~ Missing GitHub Apps APIs

StanleyGoldman requested a review from ryangribble

August 7, 2018 16:56

StanleyGoldman force-pushed the installations-for-user branch from 8acddd0 to b446b5a Compare

August 7, 2018 19:19


          Rename method

87c2022

StanleyGoldman force-pushed the installations-for-user branch from b446b5a to 87c2022 Compare

August 7, 2018 19:20

StanleyGoldman added 3 commits

August 7, 2018 15:27


          Adding installation methods

6c2e53b


          Adding tests

e94b955


          Adding observable client methods

b4b1fe8

StanleyGoldman force-pushed the installations-for-user branch from 1e5d95f to b4b1fe8 Compare

August 7, 2018 19:38

StanleyGoldman changed the title ~~Missing GitHub Apps APIs~~ Additional GitHub Apps APIs

Contributor

ryangribble commented Aug 8, 2018

👀

Contributor Author

StanleyGoldman commented Aug 8, 2018 •

edited

Loading

I'm still dogfooding these.
The documentation on these APIs kind of leave me a bit confused.
So yea, I need to actually test them all in order to understand what data they are really getting.

Contributor Author

StanleyGoldman commented Aug 8, 2018

I'm noticing a nuanced difference in the way some responses are delivered that is throwing this off..

https://developer.github.com/v3/apps/installations/#response

https://developer.github.com/v3/repos/#response

Contributor Author

StanleyGoldman commented Aug 8, 2018

cc: @shiftkey ☝️

Contributor Author

StanleyGoldman commented Aug 9, 2018

Nvm, I learned things today...


          Adding InstallationResponse

f6bc059

Member

shiftkey commented Aug 9, 2018

@StanleyGoldman is this related to the total_count field (that feels a response from a search API, but /apps/installations/ doesn't seem to behave like one) or was there some other questions/curiosities you had?

Contributor Author

StanleyGoldman commented Aug 9, 2018

Yea, I was just mentally unprepared for things to be different between api calls like that.


          Adding GitHub Apps Installations Client

40bd9dc

StanleyGoldman force-pushed the installations-for-user branch from 4b6cd2c to 40bd9dc Compare

August 9, 2018 22:04


          Tweaking doc comments

9f72740

ryangribble suggested changes

View reviewed changes

Contributor

ryangribble left a comment •

edited

Loading

Hey @StanleyGoldman it's looking pretty good, ive made some reviwe comments/thoughts just on the Observable client since that was first in the list, but the comments apply to the regular client too.

In general

I havent marked up every XMlDoc inconsistency - we will need to get the comments and param entries right, but we can focus on that once we have the main code etc done
we should think about the naming of the mehtods corrresponding to "find" endpoints as im not sure Getxxx is the best for those. I suggested a couple of options - which do you think sounds better?
since the authentication for github apps, installations and user-to-server oauth is so complex, we probably need to indicate in the XmlDoc comments of every endpoint what type of auth it requires
any endpoint taking repo /repos/:owner/:name we also by convention, support the "undocumented" endpoint /repositories/:id/

Also im not sure of the historic reasons but the main client and interface for both the Observable and normal clients live in the top level folder with all sub clients in the Clients folder below there. I see you renamed/moved the Observable one into the Clients folder but didnt do the same thing for the normal client. In general id say we shouldnt move things since it makes tracing history more difficult, unless its a really good reason... and if you think it IS a good reason, we should at least be consistent and do it for normal and observable 😁

Octokit.Reactive/Clients/IObservableGitHubAppsClient.cs Outdated

+                      /// <remarks>
+                      /// Refer to the API documentation for more information: https://developer.github.com/v3/apps/installations/
+                      /// </remarks>
+                      IObservableGitHubAppsInstallationsClient Installations { get; }

Contributor

ryangribble Aug 10, 2018

The property name should be singular, eg github.App.Installation.xxxx

Octokit.Reactive/Clients/IObservableGitHubAppsClient.cs Outdated

+                      /// <remarks>https://developer.github.com/v3/apps/#find-repository-installation</remarks>
+                      /// <param name="owner">The owner of the repo</param>
+                      /// <param name="repo">The name of the repo</param>
+                      IObservable<Installation> GetRepositoryInstallation(string owner, string repo);

Contributor

ryangribble Aug 10, 2018

I wonder if this should be named GetRepositoryInstallationForCurrent() since it uses the currently authenticated GitHub App... or otherwise FindRepositoryInstallation()?

Contributor

ryangribble Aug 10, 2018

also we need to support the repositoryId version of this call in addition to the owner/repo version

Octokit.Reactive/Clients/IObservableGitHubAppsClient.cs Outdated

+                      IObservable<Installation> GetRepositoryInstallation(string owner, string repo);
+                      /// <summary>
+                      /// Enables an authenticated GitHub App to find the repository's installation information.

Contributor

ryangribble Aug 10, 2018

comment says repository but should be organization

Octokit.Reactive/Clients/IObservableGitHubAppsClient.cs Outdated

+                      /// Enables an authenticated GitHub App to find the repository's installation information.
+                      /// </summary>
+                      /// <remarks>https://developer.github.com/v3/apps/#find-organization-installation</remarks>
+                      /// <param name="owner">The owner of the repo</param>

Contributor

ryangribble Aug 10, 2018

incorrect params

Octokit.Reactive/Clients/IObservableGitHubAppsClient.cs Outdated

+                      /// <remarks>https://developer.github.com/v3/apps/#find-organization-installation</remarks>
+                      /// <param name="owner">The owner of the repo</param>
+                      /// <param name="repo">The name of the repo</param>
+                      IObservable<Installation> GetOrganizationInstallation(string organization);

Contributor

ryangribble Aug 10, 2018

GetOrganizationInstallationForCurrent or FindOrganizationInstallation?

Octokit.Reactive/Clients/IObservableGitHubAppsClient.cs Outdated

+                      /// <remarks>https://developer.github.com/v3/apps/#find-user-installation</remarks>
+                      /// <param name="owner">The owner of the repo</param>
+                      /// <param name="repo">The name of the repo</param>
+                      IObservable<Installation> GetUserInstallation(string user);

Contributor

ryangribble Aug 10, 2018

GetUserInstallationForCurrent ? Or FindUserInstallation?

Octokit.Reactive/Clients/IObservableInstallationsClient.cs Outdated

+                  public interface IObservableGitHubAppsInstallationsClient
+                  {
+                      /// <summary>
+                      /// List repositories of the authenticated GitHub App (requires GitHubApp JWT token auth).

Contributor

ryangribble Aug 10, 2018

The docs says this endpoint uses an installation token, not a JWT... and this would get repositories of an installation rather than a github app (which would have many installations each with potentially many repos)

So let's go with "List repositories of the authenticated installation (requires Installation Token auth)" or something similar

Octokit.Reactive/Clients/IObservableInstallationsClient.cs Outdated

+                      IObservable<RepositoriesResponse> GetAllRepositoriesForCurrent();
+                      /// <summary>
+                      /// List repositories of the authenticated GitHub App (requires GitHubApp JWT token auth).

Contributor

ryangribble Aug 10, 2018

same as above

Octokit.Reactive/Clients/IObservableInstallationsClient.cs Outdated

+                      IObservable<RepositoriesResponse> GetAllRepositoriesForCurrent(ApiOptions options);
+                      /// <summary>
+                      /// List repositories accessible to the user for an installation.

Contributor

ryangribble Aug 10, 2018

Comment probably needs to indicate this requires a user to server OAuth token


          Undoing unintentional changes and cleaning up

731bd2b

StanleyGoldman force-pushed the installations-for-user branch from e3e1ade to 731bd2b Compare

August 13, 2018 19:58

Contributor Author

StanleyGoldman commented Aug 13, 2018

First off, sorry about my sloppiness, I was rushing so I can go and dogfood this. Thanks for reviewing.

I'm gonna start up a new pull request that targets this one, and I'll go through all of the GitHub Apps docs and make sure they are correct and link out correctly.
The ForCurrent moniker resonates better with me for some reason. I'll also wait for ☝️ to make sure they are consistent.
I'll make sure the docs I add have relevant links to the auth pages in the api.
Are there any other undocumented patters I should be looking for? 😼


          Reordering functions in clients to keep the same code look and feel

fc1461b

StanleyGoldman force-pushed the installations-for-user branch from 579b1d7 to fc1461b Compare

August 13, 2018 20:43


          Making sure all methods are documented with their authentication requ…

09c5021

…irements

Contributor Author

StanleyGoldman commented Aug 13, 2018

I was gonna send the latest changes in a separate pull request, but then i got lazy.


          rename GitHubAppsInstallationsClient to GitHubAppInstallationsClient …

ddeb98f

…to be consistent with single/plural naming conventions

Contributor Author

StanleyGoldman commented Aug 22, 2018

Your changes so far get a 👍 from me.

ryangribble added 2 commits

August 23, 2018 20:10


          make method order match the order on github docs site

af68a66


          tidy up usings

17d1ecb

Contributor

ryangribble commented Aug 23, 2018

I think the last thing to do is unit tests for the observable client and ideally integration tests (at least for the things we can easily setup)

ryangribble added this to the GitHub Apps milestone

ryangribble added 10 commits

August 29, 2018 18:34


          correct implementation of GetALlInstallationsForCurrent method to be …

f2c8f28

…consistent


          Add missing unit and integration tests for ObservableGitHubAppsClient

950b8b6


          fix renamed method in observable tests

5e5da13


          Add EnsuresNonEmptyArguments tests and fixup asserts in GitHubAppsClient

4a7417b


          Add tests for new Observable client methods and fixup Null/Empty asse…

bc06581

…rts in Observable client


          change non paginated call to call through to other method but with Ap…

5ea3bcf

…iOptions.None


          add unit tests for observable client and fixup errors they found

27ef381


          add integration tests for new GitHubAppsClient methods, fixed an inco…

9effc1d

…rrect route that the tests found!


          add integration tests for extra methods on observable client

bb53032


          add integration tests for new clients GitHubAppInstallationsClient an…

86d4514

…d ObservableGitHubAppInstallationsClient

Contributor

ryangribble commented Aug 29, 2018

OK I think im done with adding unit and integration tests for EVERYTHING. Glad I did this too, as it found a few problems! 😀

ryangribble approved these changes

View reviewed changes

Contributor

ryangribble commented Aug 30, 2018

Hey @StanleyGoldman these latest changes I already had a separate PR for #1857 and I also did them in a way that keeps the old models working for GHE 2.14 since it won't get these changes

I think I did forget to add the start/end column members but we can just add that to the other PR and keep this one focused on the app/installation endpoints

StanleyGoldman force-pushed the installations-for-user branch from e71cc85 to 86d4514 Compare

August 30, 2018 22:02

Contributor Author

StanleyGoldman commented Aug 30, 2018

Ah my bad, I was not aware of the nuance.

ryangribble added the category: feature label

Contributor

ryangribble commented Sep 3, 2018

release_notes: Implement additional endpoints for GitHub Apps to find installations for a given organization, repository or user

Contributor

ryangribble commented Sep 3, 2018

release_notes: Implement GitHub Apps Installation API to allow listing all repositories a GitHub App Installation or GitHub App authenticated user has access to


          deprecate renamed method properly, to avoid breaking change

b2b28bc

Contributor

ryangribble commented Sep 3, 2018

I think these are good to go!

ryangribble merged commit 5f1421b into master

ryangribble deleted the installations-for-user branch

September 3, 2018 10:36

nickfloyd added Type: Feature and removed category: feature labels

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels