octokit.net icon indicating copy to clipboard operation
octokit.net copied to clipboard

Add deliveries api

Open darksidersstrife opened this issue 3 years ago • 3 comments

Github has api for getting webhook deliveries and redelivering them (for repos, orgs and apps) I'm willing to implement it (at least for apps, even though repos and orgs api is identical, i'm not sure if i have time to write three clients with tests for each), but i have some questions on how would you want me to do it

  1. Pagination used in this api differs from the usual (cursor is used instead of a page number and also there are some links differences) Would you like to:
    1. have separate set of methods in ApiConnection with an argument of new class for this pagination (same as ApiOptions, but with a cursor instead of page number), and so new api will use it too (a lot of copy pasting, but easy)
    2. have cursor added to ApiOptions and to paginate differently based on it's presence (there are a lot of common parts in these two pagination types, so it will be a slight modification, but kind of an ugly one i guess)
    3. refactor pagination to support both types (good, but ApiPagination, IReadOnlyPagedCollection and a lot of other classes, which are needed to be changed, are public, even though they are to be used internally, so it would be kind of a breaking change)
  2. Deliveries returned by endpoint that lists them do not include payload and header, but ones returned by id do. I've tried to look how this problem is solved for other endpoints, but didn't find much - e.g. response of querying a branch by name includes it's protection rules, while branches in response for a list endpoint do not, but in octokit protection rules in response are just ignored and same model is used For deliveries payload and headers cannot be ignored, so should i create two separate models or have those fields as nullable in one model (which is misleading) or have one model inherit another (not sure if it wouldn't introduce some problems with deserialising, or maybe it's just conceptually wrong)

darksidersstrife avatar Aug 31 '22 01:08 darksidersstrife

@darksidersstrife Thanks for reaching out and for your willingness to implement this API in Octokit.net - we really appreciate it ❤️

GitHub's resident .NET expert, @nickfloyd is on vacation until Tuesday, but he'll be able to help on this when he's back - or perhaps someone from the community like @JonruAlveus can help.

timrogers avatar Sep 02 '22 12:09 timrogers

Hello! I would go for 1.i, it keeps it nice and simple and obvious about what's going on. Changing the existing ApiOptions makes ApiOptions a little unsure about it's purpose and we want to keep these changes as small as possible.

With regards to 2, I think create two separate (if similar) classes. The nullable properties are misleading and will likely lead to null reference exceptions when someone gets the list and expects all of the child items to be there.

JonruAlveus avatar Sep 04 '22 08:09 JonruAlveus

Hey @darksidersstrife, thanks for bringing this up and being available to build things out. Have you had a look at https://github.com/octokit/webhooks.net yet (same with GraphQL)? I know @JamieMagee and others have done a lot of great work around webhooks in that implementation. I'd like to keep the REST implementation and webhooks logic distinct for now while we work toward generative SDKs (using our OpenAPI descriptions) if possible.

That aside, if you have thoughts on why the two protocols should exist here, let's take some time to chat about that. It would be great to hear your perspective and thoughts. Additionally, I might've misinterpreted what you'd like to do here, so please let me know.

nickfloyd avatar Sep 13 '22 19:09 nickfloyd

Hey @darksidersstrife,

I'm going to close this as it's now over a year old. If you still need this addressed, please feel free to reopen the issue.

nickfloyd avatar Oct 16 '23 21:10 nickfloyd

Hi @nickfloyd,

As far as I can tell, the project you linked (webhooks.net) is concerned with being on the receiving end of a webhook request, not so much about querying the GitHub API.

From what I gather from the opening post, @darksidersstrife wanted to implement the client methods for the endpoints at GitHub, i.e. the following endpoints: https://docs.github.com/en/rest/repos/webhooks?apiVersion=2022-11-28#list-deliveries-for-a-repository-webhook https://docs.github.com/en/rest/repos/webhooks?apiVersion=2022-11-28#get-a-delivery-for-a-repository-webhook https://docs.github.com/en/rest/repos/webhooks?apiVersion=2022-11-28#redeliver-a-delivery-for-a-repository-webhook and the corresponding ones for Apps and Organizations.

jvmdc avatar Jul 18 '24 12:07 jvmdc