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

feat: Support --header for CLI commands to support proxies #4008

Merged
merged 1 commit into from
Sep 12, 2022
Merged

Conversation

kylecarbs
Copy link
Member

Fixes #3527.

@kylecarbs kylecarbs self-assigned this Sep 12, 2022
Copy link
Collaborator

@sreya sreya left a comment

Choose a reason for hiding this comment

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

Seems fine but wouldn't this make more sense being integrated in the sdk? You could just add a variadic options parameter to codersdk.New and modify the transport on the HTTP client there.

@kylecarbs
Copy link
Member Author

I didn't want to abstract the client just for this change, because it seems like a weird one.

@sreya
Copy link
Collaborator

sreya commented Sep 12, 2022

Based on the issue though isn't the expectation that all commands accept an arbitrary list of headers?

@kylecarbs
Copy link
Member Author

Yup, this is a global flag.

@sreya
Copy link
Collaborator

sreya commented Sep 12, 2022

Like I said it's fine but it's just weird we're introducing a new way that every command needs to instantiate a client. It's technically a bug to call codersdk.New directly now in cli. Also if any third party wants to use codersdk as a library they lose out on this functionality and have to write a similar wrapper.

Also unless I'm missing something this PR only implements arbitrary headers for login is there some reason we're not addressing the rest of the commands in this PR?

@kylecarbs
Copy link
Member Author

Nah, this just tests it against login but we could test it against arbitrary commands.

My argument for not exposing this functionality is that it's pretty weird and also trivial for someone else to implement if they want to do so.

@kylecarbs kylecarbs merged commit a209825 into main Sep 12, 2022
@kylecarbs kylecarbs deleted the cliheader branch September 12, 2022 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support --header for CLI commands to support CloudFlare Access or other identify-aware proxies
2 participants