Skip to content
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

fix: add webhook deliveries APIs #285

Merged
merged 4 commits into from
Jun 28, 2021
Merged

fix: add webhook deliveries APIs #285

merged 4 commits into from
Jun 28, 2021

Conversation

kammerjaeger
Copy link
Contributor

@kammerjaeger kammerjaeger commented May 8, 2021

Hi,

I'm trying to use the new event redelivery api
The request are not working for me because it seams to want to use the installation token when I use the endpoint:
app/hook/deliveries?status=failure

I tried it from a probot app with:

const octokit = await app.auth();
const result2 = await octokit.request("GET /app/hook/deliveries", { status: "failure", mediaType: { previews: [mediaPreview] } });

I'm not sure about the other apis, if they need an installation token or an app bearer.
What do you think?

@gr2m
Copy link
Contributor

gr2m commented May 11, 2021

That's a tricky one, these endpoints are not documented yet, they seem to be tested with a select group of people. Can you make sure it's okay to share this gist in the first place?

I have to think about this for a moment, I don't want to add routes to the source code that might still change, because that would cause an unnecessary breaking change. Also these routes might be for internal testing, not meant to be made public yet.

I think the right thing to do would be to add an option to pass additional routes that require the app authentication, but I have to think about it for a moment

@gr2m gr2m self-assigned this May 11, 2021
@kammerjaeger
Copy link
Contributor Author

I agree, your last proposal sounds to me to be the best solution. I tried to somehow add the route without this PR (because of the beta) but could not find any way to do it.
Also the error message that a installation token is needed should maybe then have a line to say that: if it is new api and needs the app auth add it that way.

@gr2m gr2m added the Type: Bug Something isn't working as documented, or is being fixed label May 12, 2021
@gr2m gr2m added Type: Feature New feature or request and removed Type: Bug Something isn't working as documented, or is being fixed labels Jun 27, 2021
@gr2m gr2m changed the title Add missing entry for new redelivery api Set JWT authentication on custom routes Jun 27, 2021
@gr2m gr2m changed the title Set JWT authentication on custom routes feat: set JWT authentication on custom routes Jun 27, 2021
@gr2m gr2m changed the title feat: set JWT authentication on custom routes fix: add webhook deliveries APIs Jun 27, 2021
src/requires-app-auth.ts Show resolved Hide resolved
@gr2m gr2m removed their assignment Jun 27, 2021
Co-authored-by: Gregor Martynus <39992+gr2m@users.noreply.github.com>
@kammerjaeger
Copy link
Contributor Author

Thanks for the other two paths. I totally did not realize that was also needed.

@gr2m gr2m merged commit b567472 into octokit:master Jun 28, 2021
@github-actions
Copy link
Contributor

🎉 This PR is included in version 3.5.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants