Use requests' json argument when passing JSON to the taskcluster API#415
Merged
Conversation
When triggering hooks, the previous code was serializing the JSON itself then passing it straight to taskcluster as a string with no content type. This currently works because the taskcluster proxy has a workaround (see taskcluster/taskcluster#3521) that sets the content type to application/json if it's missing from a request with a non empty body. This means that if that workaround was ever removed or if this script tries to target taskcluster directly without the proxy, the request would fail. Just use `requests.post(json=` and get serialization and the header for free. The manual json.dumps() and the \n to space replace were leftovers from when the payload was shelled out through `echo -n '...' | taskcluster-darwin-amd64` in the past, where embedded newlines broke the quoted shell string.
Eijebong
added a commit
to Eijebong/taskcluster
that referenced
this pull request
Apr 22, 2026
This removes a years old workaround for misbehaving clients. The workaround was added in ef7164f (taskcluster#3671) to restore some behavior that was accidentally dropped in f3c2473 when the proxy got put in the monorepo. Before that move, the proxy was delegating outgoing requests to `tcclient.Client.Request` which was unconditionally setting `Content-Type` for any non-empty body. The monorepo version is calling `http.NewRequest` directly which does not have that kind of behavior (so the header disappeared). I think it's time to let this debt go and do what was decided in taskcluster#3521, which is to not touch headers in the proxy. I think it makes no sense to try and fix a buggy client by hiding its behavior since it might also be run directly talking to taskcluster and not through the proxy. Worth noting, I checked papertrail for that log line and found one offending client for which I opened a PR (mozilla-services/updatebot#415). Closes taskcluster#3521
Eijebong
added a commit
to Eijebong/taskcluster
that referenced
this pull request
Apr 22, 2026
This removes a years old workaround for misbehaving clients. The workaround was added in ef7164f (taskcluster#3671) to restore some behavior that was accidentally dropped in f3c2473 when the proxy got put in the monorepo. Before that move, the proxy was delegating outgoing requests to `tcclient.Client.Request` which was unconditionally setting `Content-Type` for any non-empty body. The monorepo version is calling `http.NewRequest` directly which does not have that kind of behavior (so the header disappeared). I think it's time to let this debt go and do what was decided in taskcluster#3521, which is to not touch headers in the proxy. I think it makes no sense to try and fix a buggy client by hiding its behavior since it might also be run directly talking to taskcluster and not through the proxy. Worth noting, I checked papertrail for that log line and found one offending client for which I opened a PR (mozilla-services/updatebot#415). Closes taskcluster#3521
|
@tomrittervg hi 👋 Would you have some spare cycle to look at this PR? The taskcluster-proxy will soon be upgraded to the Firefox CI instance and will break without this PR. |
tomrittervg
approved these changes
May 20, 2026
Collaborator
|
Sorry for the delay |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When triggering hooks, the previous code was serializing the JSON itself then passing it straight to taskcluster as a string with no content type. This currently works because the taskcluster proxy has a workaround (see taskcluster/taskcluster#3521) that sets the content type to application/json if it's missing from a request with a non empty body. This means that if that workaround was ever removed or if this script tries to target taskcluster directly without the proxy, the request would fail. Just use
requests.post(json=and get serialization and the header for free.The manual json.dumps() and the \n to space replace were leftovers from when the payload was shelled out through
echo -n '...' | taskcluster-darwin-amd64in the past, where embedded newlines broke the quoted shell string.