- Notifications
You must be signed in to change notification settings - Fork1k
feat: Support--header for CLI commands to support proxies#4008
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
Seems fine but wouldn't this make more sense being integrated in the sdk? You could just add a variadic options parameter tocodersdk.New and modify the transport on the HTTP client there.
I didn't want to abstract the client just for this change, because it seems like a weird one. |
Based on the issue though isn't the expectation that all commands accept an arbitrary list of headers? |
Yup, this is a global flag. |
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 Also unless I'm missing something this PR only implements arbitrary headers for |
Nah, this just tests it against 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. |
Fixes#3527.