-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Adding credential-authorization get endpoints #2556
Conversation
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.
@JonruAlveus This is another great addition! Thank you ❤️ - would you mind running the async pagination generators to ensure this and other newly added APIs are covered on the paging front?
dotnet run --project Octokit.Generators
I am planning on integrating this in the build command or even in CI, but I wanted to have it run manually so that we (the humans) could validate that it was doing what it is supposed to do.
Welcome back :) Thanks for the review, I've run the update and there appears to be a large diff because the new class has the method in alphabetical order, which it wasn't before. |
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.
Sorry I missed a couple of items regarding method comments in the ApiUtil
@@ -4457,5 +4457,15 @@ public static Uri Meta() | |||
{ | |||
return "meta".FormatUri(); | |||
} | |||
|
|||
public static Uri AllOrganizationCredentials(string org) |
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.
Would you also mind adding method comments here? Sorry I completely missed this during the initial review.
return "orgs/{0}/credential-authorizations".FormatUri(org); | ||
} | ||
|
||
public static Uri AllOrganizationCredentials(string org, string login) |
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.
Same as with public static Uri AllOrganizationCredentials(string org)
@JonruAlveus Let me know if you'd like me to knock these out if you don't have the time. I'd like to get these changes in for the next release and would be glad to take them over if you are strapped on bandwidth. I'll need push access to your fork. Let me know when you get the chance! |
@nickfloyd Hi! I’m currently overseas so I’ve added you as a collaborator on my fork. Feel free to knock these out :) I’m not even going to attempt coding on an iPad 🤣 |
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.
👍 LGTM!
release_notes: Adds credential-authorization get endpoints |
U crazy |
fixes #2421
I do not have access to a github enterprise account for testing, so I didn't write any integration tests.