search

google / oauth2l

oauth2l ("oauth tool") is a simple CLI for interacting with Google API authentication.
Apache License 2.0
651 stars 81 forks source link

curl stdout goes to oauth2l stderr #146

Closed timdp closed 1 year ago

timdp commented 1 year ago

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

https://github.com/google/oauth2l/blob/2105d6b0f3781e4ef97e1dd9c513f636dd721982/util/curl.go#L42

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.