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

curl stdout goes to oauth2l stderr #146

Closed
timdp opened this issue Jan 3, 2023 · 3 comments · Fixed by #147
Closed

curl stdout goes to oauth2l stderr #146

timdp opened this issue Jan 3, 2023 · 3 comments · Fixed by #147

Comments

@timdp
Copy link
Contributor

timdp commented Jan 3, 2023

In curl.go, the output of the curl command gets fed into Go's print:

print(out.String())

but that sends it to stderr, and it's not considered all that stable:

https://github.com/golang/go/blob/db36eca33c389871b132ffb1a84fd534a349e8d8/src/builtin/builtin.go#L265-L269

Isn't oauth2l curl mostly supposed to be a drop-in wrapper for curl? If so, I would expect the invocation to inherit the program's channels so that stdout and stderr can use streaming. Similarly, this would presumably also unlock streaming stdin for POST and friends.

Additionally, the previous statement writes any errors to stdout, so the channels are in fact getting swapped. This is confusing, no?

@andyrzhao
Copy link
Collaborator

Hi Tim, thank you for your suggestion! "oauth2l curl" was added mostly to simplify the syntax for invoking API calls, and I did not pay attention to the output (stderr vs stdout) when implementing it. In fact, I do not even remember why the built-in "print" was used over the fmt version - this is likely a mistake. Feel free to put up a PR to correct the channels - however, this would be considered a "breaking" change. We should document the change well, especially considering errors else-where are being written to stdout today - and it may be useful if you can provide a concrete example of how to take advantage of the updated channels. We could include the change in the next major release. Thanks!

@timdp
Copy link
Contributor Author

timdp commented Jan 3, 2023

I also thought it was a mistake, but I wanted to check first. Go stdio isn't that intuitive.

I was thinking the corrected behavior could be behind a flag, but that would complicate things. I would indeed rather just release this as a major version.

I'll see if I can come up with a PR!

@timdp
Copy link
Contributor Author

timdp commented Jan 4, 2023

If we're making a breaking change anyway, maybe it would make sense to make the oauth2l-curl combo look more like sudo?

That way, one could prefix any curl command (for a relevant URL) with oauth2l + its flags to authenticate it, without having to alter the unauthenticated command any further. For example:

oauth2l --scope cloud-platform \
  curl -fsSL https://cloudfunctions.googleapis.com/v2/...

It would probably also be trivial to get the same functionality in place for wget and httpie, to name but a few. Note that it wouldn't work for arbitrary commands (like sudo) because it does involve changing the wrapped command.

I imagine this would entail a more invasive change to the argument parser though.

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 a pull request may close this issue.

2 participants