-
-
Notifications
You must be signed in to change notification settings - Fork 276
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
Partial implementation of Organization CoPilot APIs #747
base: main
Are you sure you want to change the base?
Conversation
#[serde(skip_serializing_if = "Option::is_none")] | ||
per_page: Option<u8>, | ||
#[serde(skip_serializing_if = "Option::is_none")] | ||
page: Option<u32>, |
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.
I have not yet found a reason for pagination unless someone sets per_page
to a lower amount. Feels very unnecessary but I kept it by the book: Usage and Metrics endpoints support pagination, billing does not. https://docs.github.com/en/rest/copilot/copilot-usage?apiVersion=2022-11-28
} | ||
|
||
// Retrieve copilot metrics for a specific team within the organization | ||
pub async fn metrics_team<T: ToString>( |
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.
The _team
functions have a generic <T: ToString>
so you can pass both a &'a str
(in most cases, people only look for one organization and it's most likely 'static
) as well as a String
.
Most APIs in the codebase seemed to want a String but it didn't make sense to me to take something that is relatively constant, so this is more developer sugar than anything else.
for safety reasons, i will assume the others are also nullable, since the docs claim they are not
I tested this against real data and fixed the bugs/inconsistencies with their docs 👍🏻 some fields were nullable when docs suggested they weren't 😕 |
I am an enterprise administrator for our organization's GitHub instance, we use copilot at our organization. I've been developing software that uses these metrics and seats but the library did not yet support the endpoints I required.
This partially tackles #525
I have done a best effort of implementing the stuff I needed, I have left comments on things I did not [yet] implement. This MR is considered ready (especially for review), but the copilot section itself is considered not fully implemented.
The testsuite tests against data GitHub gives but their samples are not entirely feature complete. It is possible I missed a nullable somewhere. Across this week, I will be testing against real data using my fork, if you'd like to wait until I've narrowed down the bugs I could find I'd advocate waiting until the testing checkboxes at the bottom is checked, but I'm also happy to submit follow up PRs to fixes if required.
Implementation
Checkboxes serve as a guide / overview, it is not intended for them to reach 100% checked.
https://docs.github.com/en/rest/copilot?apiVersion=2022-11-28
Implementation Notes
1: there is an interesting "API design" from GitHub where their json is effectively like this:
There is no reason I observed to wrap this into an one-element array, this means the code also looks kind of ugly:
usage
andmetrics
APIs return aVec<Vec<Struct>>
, because the documentation says they can be paginated but realistically nobody will. It's kind of awkward but I opted to translate literally and suggest library consumers make prettier wrappers around it. As the copilot API is changing from time to time and some are marked experimental (e.g. usage) trying to fix their issues.2:
Copilot usage APIs are experimental;
Testing progress:
page
,page_amount
,until
,since
)Note that even if I tested against real data, it does not guarantee perfect infallibility. We are only human :)
Disclosure
It is worth noting that some of this code was generated using GitHub Copilot, in particular the struct boilerplate of the models after I copy/pasted the API response as a comment. If the maintainers of this project would rather not have GenAI generated code in your project I encourage declining this PR. Those exact copy/paste's are visible in the test
.json
files.