-
-
Notifications
You must be signed in to change notification settings - Fork 598
add the ability to fetch repositories for a specific installation and user #578
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 the ability to fetch repositories for a specific installation and user #578
Conversation
lib/Github/Api/CurrentUser.php
Outdated
*/ | ||
public function repositoriesByInstallation($installationId, array $params = array()) | ||
{ | ||
return $this->get("/user/installations/$installationId/repositories", array_merge(array('page' => 1), $params)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is page hardcoded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Page is only hardcoded when you don't pass a page array key in the params array. So this is the default value
It looks like the styleci job for this PR was not started. You could rebase and force push which should trigger a new build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good.
You also need to add a configure method to set Accept header to application/vnd.github.machine-man-preview+json
lib/Github/Api/CurrentUser.php
Outdated
*/ | ||
public function repositoriesByInstallation($installationId, array $params = array()) | ||
{ | ||
return $this->get("/user/installations/$installationId/repositories", array_merge(array('page' => 1), $params)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets use sprintf here. Also, do single quotes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
@Nyholm I'm not sure about your last comment (the Accept header). I think it's outside of the scope of this PR. If I want to call any route regarding integrations (any method in I see two benefits of not adding it here:
|
I did not know. Thanks.
I do agree with your reasons, but the configure method is needed for consistency.
Yeah, since it is needed for the existing endpoints as well. I created a new issue for that #580 |
Thank you for this PR. |
https://developer.github.com/early-access/integrations/user-identification-authorization/#check-which-installations-resources-a-user-can-access
@Nyholm @acrobat