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

Set blank headers so they are not automatically resolved by go-gh #5935

Merged
merged 2 commits into from Jul 14, 2022

Conversation

samcoe
Copy link
Contributor

@samcoe samcoe commented Jul 13, 2022

This PR fixes two bugs dealing with the handling of headers for http requests.

  1. Requests using an api.Client will now properly send the "User-Agent" header instead of having it overwritten by go-gh.
  2. Requests from the api command can pass an alternative Authorization header.

@samcoe samcoe self-assigned this Jul 13, 2022
@csainty
Copy link

csainty commented Jul 13, 2022

I've got a problem today where I can no longer pass an alternative Authorization header to gh api calls. It is ignored and my logged in user is used instead.

Am I right in thinking this PR would resolve that?

@Lol437

This comment was marked as spam.

@samcoe samcoe marked this pull request as ready for review July 14, 2022 07:02
@samcoe samcoe requested a review from a team as a code owner July 14, 2022 07:02
@samcoe samcoe requested review from mislav and removed request for a team July 14, 2022 07:02
@cliAutomation cliAutomation added this to Needs review 🤔 in The GitHub CLI Jul 14, 2022
Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed that this fixes things for me; thank you! In the future, I propose that we make this more declarative; i.e. that we don't have to explicitly set headers to blank strings just so that they can be set to a correct value later. But I guess that might require a change to go-gh behavior?

authorization: "",
contentType: "",
timeZone: "",
userAgent: "",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The semantics for passing a blank string to get the desired (non-blank) result in the end is confusing to me; would there be a way to make this clearer as follow-up?

The GitHub CLI automation moved this from Needs review 🤔 to Needs to be merged 🎉 Jul 14, 2022
@samcoe
Copy link
Contributor Author

samcoe commented Jul 14, 2022

@mislav I agree it is confusing. I have a few ideas on what we could do in go-gh to make this better. I think making the changes here are be the quickest solution to fix the bug and once the bug is fixed we can take a more thorough look at the options we have to make it less confusing.

@samcoe samcoe merged commit 9d70d62 into trunk Jul 14, 2022
The GitHub CLI automation moved this from Needs to be merged 🎉 to Pending Release 🥚 Jul 14, 2022
@samcoe samcoe deleted the fix-user-agent branch July 14, 2022 12:13
@github-actions github-actions bot moved this from Pending Release 🥚 to Done 💤 in The GitHub CLI Jul 14, 2022
@1yahyah

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
The GitHub CLI
  
Done 💤
Development

Successfully merging this pull request may close these issues.

None yet

5 participants