-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Fix Webhook creation. #1117
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 Webhook creation. #1117
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
I signed it! |
CLAs look good, thanks! |
The only documentation I found about this: Do you want to contact GitHub ([email protected]) and ask them to clarify what is going on after describing the situation you discovered? I have found their support team to be incredibly responsive and helpful. |
I have just emailed GitHub support about this, will update here once I get response from them. |
Here's a response I received from a helpful GitHub support folk: This field is currently a legacy field, in that we only provide a single option for this: 'web' for webhooks. Previously we had the concept of GitHub Services: https://developer.github.com/changes/2018-10-01-denying-new-github-services/ that allowed you to create hooks for this. However as mentioned in the above article we have since deprecated and switched off this service. We decided to leave this field in, to prevent issues with current integrations that might still rely on this. In this case it automatically defaults to 'web' if omitted, or if provided, must have the value 'web'. For instance the following core curl request works:
Where $TOKEN is the OAuth token, and $URL is the resource to send the requests to. Long term we could choose to add other values to this as we are often iterating on the services that we provide, we could also choose to in the long term officially deprecate and remove this field as part of an ongoing effort to improve our validation rules. In terms of the PR, #1117, the author @dominicgunn mentions that:
As we haven't been able to reproduce this with the above curl I'd be interested to know if the user can reproduce this with a TL;DR; This field has a single value, is a legacy field, and has a default value if omitted. In terms of defining this field in the SDK, I feel like the SDK should emulate the API behaviour:
I'm not sure if the SDK has any convention around validation of arguments, which may need to be added. |
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.
Thank you, @dominicgunn, and thank you, @radeksimko for contacting GitHub support and reporting back!
I have just one minor tweak, and then we can merge after we get a second LGTM.
Note that this PR does not implement the recommendation in the above comment but it sounds to me like the API is not changing, so this solution would solve the issue and is fine with me. We'll see if @gauntface agrees. |
For some deeper clarity, this is against a enterprise version of Github (GHE), I'm not sure how much difference that would make. In terms of allowing this to be definable by the user, I'm not against it but it feels superfluous given that the only accepted value will be "web". Thanks @radeksimko for reaching out to Github support, I'm sorry I haven't been more involved in the discussion here. |
I'm ok with this landing, but it does sound like a bug in GHE if you are getting an error without the name field. |
Having it definable would help us a lot in the Terraform GitHub provider as we still haven't upgraded SDK since this field was removed and if the field is put back we can deprecate it more gracefully and easily, rather than just pulling the cord and forcing everyone to change their configurations. Also we might actually not deprecate it at all as there might be some plans in the future around that field as the GitHub Support folk mentioned. I'm willing to PR that change separately though. |
Hi All, |
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.
Thank you, everyone, for your comments. This should help GitHub Enterprise users.
LGTM.
Merging.
Changes
Attempting to create webhooks fail unless a name is specified. As per the documentation the only name that is valid is "web", so I haven't opted to allow modification of this but creation seems to fail without it.
This seems to impact only Github Enterprise.