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

json output format should NOT output progress #644

Closed
LarsKumbier opened this issue Dec 18, 2023 · 11 comments · Fixed by #656 or #664
Closed

json output format should NOT output progress #644

LarsKumbier opened this issue Dec 18, 2023 · 11 comments · Fixed by #656 or #664

Comments

@LarsKumbier
Copy link

LarsKumbier commented Dec 18, 2023

hcloud server request-console <server> -o json now outputs not only json, but also the progress on the STDERR channel:

$ hcloud server request-console <server> -o json
1.1s [===================================] 100.00%
{
  "wss_url": "wss://web-console.hetzner.cloud/?server_id=<server_id>\u0026token=<token>",
  "password": "<password>"
}

If json is requested, only json should be in the output. Maybe add an optional --progress, if that feature should still be present.

Issue was introduced with hcloud v1.41.
last known good is hcloud v1.40.

@phm07
Copy link
Contributor

phm07 commented Dec 19, 2023

This is intentional. Because the progress is printed to stderr, you can handle it separately. For example, this is how you would hide it entirely:

server request-console <server> -o json 2>/dev/null

I don't think an extra flag for that would make sense.

@jooola
Copy link
Member

jooola commented Dec 19, 2023

I think it's worth adding some details about what is printed to stdout and stderr to the documentation/help page/manpage.

@itg-dave
Copy link

itg-dave commented Dec 19, 2023

This is intentional. Because the progress is printed in stderr, you can handle it separately. For example, this is how you would hide it entirely:

server request-console -o json 2>/dev/null

I don't think an extra flag for that would make sense.

This doesn't make sense IMHO. If you provide JSON output, the output should be valid JSON and nothing else (p.e. kubectl -o yaml). This is not the usual way to provide this kind of feature. This breaks tools like jq which normally would be piped without the need to redirect stderr to /dev/null...

I can only speak for myself but this is the first application where I am forced to do it this way.

@jooola jooola added bug and removed bug labels Dec 19, 2023
@LarsKumbier
Copy link
Author

This is intentional. Because the progress is printed in stderr, you can handle it separately. For example, this is how you would hide it entirely:

That's how I solved it for now, but I also never saw a solution like that.
I'd rather see a commandline flag instead.

@itg-dave
Copy link

itg-dave commented Dec 19, 2023

This is intentional. Because the progress is printed in stderr, you can handle it separately. For example, this is how you would hide it entirely:
server request-console -o json 2>/dev/null
I don't think an extra flag for that would make sense.

This doesn't make sense IMHO. If you provide JSON output, the output should be valid JSON and nothing else (p.e. kubectl -o yaml). This is not the usual way to provide this kind of feature. This breaks tools like jq which normally would be piped without the need to redirect stderr to /dev/null...

I can only speak for myself but this is the first application where I am forced to do it this way.

Another solution could be a quiet-flag (like curl -s). If this is passed, we can ensure, that script-friendly output is generated.

@phm07
Copy link
Contributor

phm07 commented Dec 19, 2023

This is intentional. Because the progress is printed in stderr, you can handle it separately. For example, this is how you would hide it entirely:
server request-console -o json 2>/dev/null
I don't think an extra flag for that would make sense.

This doesn't make sense IMHO. If you provide JSON output, the output should be valid JSON and nothing else (p.e. kubectl -o yaml). This is not the usual way to provide this kind of feature. This breaks tools like jq which normally would be piped without the need to redirect stderr to /dev/null...

I can only speak for myself but this is the first application where I am forced to do it this way.

It doesn't break jq. A pipe only forwards stdout by default, we considered that when we made the change.
Only problem I see so far is that you can't distinguish errors (which are printed to stderr too) from the progress bar output. So I think a --quiet (opt-out) or --progress (opt-in) could be useful indeed.

@jooola jooola added the feature label Dec 19, 2023
@LarsKumbier
Copy link
Author

You're completely right; turns out it was a completely separate issue. Should I leave the issue open as a feature request nonetheless?

@itg-dave
Copy link

itg-dave commented Dec 19, 2023

It doesn't break jq. A pipe only forwards stdout by default, we considered that when we made the change. Only problem I see so far is that you can't distinguish errors (which are printed to stderr too) from the progress bar output. So I think a --quiet (opt-out) or --progress (opt-in) could be useful indeed.

Thanks for the clarification, i corrected my comment above. And thank you for considering this!

@phm07
Copy link
Contributor

phm07 commented Dec 19, 2023

Yes, I think the concern that you can't (easily) get error messages while ignoring stderr is still valid, so a --quiet flag is still useful IMO. Thanks for bringing this up for discussion 🙂

@phm07 phm07 self-assigned this Jan 4, 2024
apricote pushed a commit that referenced this issue Feb 1, 2024
## [1.42.0](v1.41.1...v1.42.0) (2024-02-01)


### Features

* add global --quiet flag to hide non-error messages
([#656](#656))
([25fcbbf](25fcbbf)),
closes [#644](#644)
* allow adding/removing multiple labels at once
([#665](#665))
([919c446](919c446)),
closes [#662](#662)
* group subcommands in command help
([#675](#675))
([0cb271f](0cb271f))
* **server:** remove unsupported linux32 rescue type
([#679](#679))
([5bb0350](5bb0350))


### Bug Fixes

* refetch after creating managed certificate
([#685](#685))
([4864553](4864553))
* **server:** fix typo in ip subcommand
([#678](#678))
([c5e3f00](c5e3f00))
* use --poll-interval flag
([#660](#660))
([b9328a6](b9328a6))
@Srakbi
Copy link

Srakbi commented Apr 2, 2024

hcloud server request-console <server> -o json now outputs not only json, but also the progress on the STDERR channel:

$ hcloud server request-console <server> -o json
1.1s [===================================] 100.00%
{
  "wss_url": "wss://web-console.hetzner.cloud/?server_id=<server_id>\u0026token=<token>",
  "password": "<password>"
}

If json is requested, only json should be in the output. Maybe add an optional --progress, if that feature should still be present.

Issue was introduced with hcloud v1.41.
last known good is hcloud v1.40.

Close _ # 644

@Srakbi
Copy link

Srakbi commented Apr 2, 2024

I'm sorry 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants