Fixed headers for EventType & DeliveryID #3554
Conversation
Apparently github webhook headers are actually have uppercase "H" in "GitHub", instead "Github": ``` map[Accept:*/* Content-Type:application/json User-Agent:GitHub-Hookshot/ef91aa7 X-GitHub-Delivery:****** X-GitHub-Event:workflow_job X-GitHub-Hook-ID:****** X-GitHub-Hook-Installation-Target-ID:****** X-GitHub-Hook-Installation-Target-Type:repository X-Hub-Signature:sha1=****** X-Hub-Signature-256:sha256=******] ```
|
According to claude.ai:
So my interpretation of this is that our casing of So unless there is a good reason to change it (which I don't think there is but am willing to listen), I think we should close this issue. |
|
Looking through the code, we have specifically chosen to make the keys canonical in #3389. |
|
Closing due to reasons listed in #3389. |
|
Hi @gmlewis, Sorry, but I should try to protect my changes here. Unfortunately the mentioned #3389 does not completely apply here: the mentioned headers in #3389 are internal for the go-github package, the changed ones in this PR are external. I would also understand if the standard will be used by GitHub here - but it's clearly is not, so what's the point of following the standard if it makes interface to not work correctly, forcing the developer to jump through hoops? But maybe I'm wrong here and using the provided interfaces incorrectly - maybe you can tell me what to do in the following case: The issue I have with those headers: when I try to parse webhook (both from github.HookDelivery & incoming webhook) I need to read the headers like that: guid, ok := headers[github.DeliveryIDHeader] // Bug here due to incorrect key
if !ok {
return fmt.Errorf("Can't find delivery GUID in headers")
}
sig, ok := headers[github.SHA256SignatureHeader]
if !ok {
return fmt.Errorf("Can't find delivery signature in headers")
}
eventType, ok := headers[github.EventTypeHeader] // Bug here due to incorrect name
if !ok {
return fmt.Errorf("Can't find delivery event type in headers")
}Since those are used on client receiving side (defined and once used in Thank you |
|
Oh, I see. You are trying to use these directly to look up the values. |
|
Have you tried this? https://pkg.go.dev/net/http#Header.Get |
|
https://pkg.go.dev/net/http#Header.Get is pretty industry-standard. |
|
The issue I have right now is with HookDelivery accessed by REST API - we have to use it to make sure no webhook requests are missing due to network issues. Unfortunately HookDelivery and it's internal HookRequest are not providing easy way to access Headers but to use map interface... |
Ah, I see what you mean. Then I would suggest a different way of fixing this. The reason I don't want to change the casing of our constant strings is not only because of the standard, but GitHub could change the capitalization of their strings every other Monday and we would have to attempt to jump through hoops to keep up-to-date with their changes. Therefore, let us fix this once and fix it such that no matter what capitalization is sent to this client library, it can handle it and work correctly. Would you like to work on what I have suggested, or would you prefer that we turn this into an issue (instead of a PR) and have someone else tackle this issue, @sparshev ? |
|
Thank you for explaining @gmlewis , appreciate your help! Yeah, let's maybe turn it into an issue then - since those header keys are interfaces for the user of the library, I think they need to be good enough to use with the library data interfaces. Maybe the HookRequest itself could implement the http.Header.Get - kind of interface... |
Apparently github webhook headers are actually have uppercase "H" in "GitHub", instead "Github":