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

healthcheck: fix toconsole condition logging #1200

Merged
merged 1 commit into from
Mar 16, 2024

Conversation

clwluvw
Copy link
Contributor

@clwluvw clwluvw commented Mar 12, 2024

Log to console either hasattr() or isatty()

@vincentbernat
Copy link
Member

This doesn't look correct. The check requires the isatty attribute to exist to call it. If I remember correctly, it may not exist on Windows, hence the check. With your change, if the attribute does not exist, you call it, which will trigger an exception. And if it exists, you don't really check this is a tty.

@clwluvw
Copy link
Contributor Author

clwluvw commented Mar 12, 2024

@vincentbernat This is now not working inside a docker container. Do you have any alternative way to solve it?

@vincentbernat
Copy link
Member

Well, I suppose nowaday, when running daemonized, stderr is set to /dev/null, so it shouldn't be a problem to write to it even when this is a not tty, so we may just replace toconsole by not silent. People can still opt out that with --silent.

@clwluvw clwluvw force-pushed the healthcheck-logging branch from 7fa3a9b to 5339a8a Compare March 12, 2024 20:37
@clwluvw
Copy link
Contributor Author

clwluvw commented Mar 12, 2024

Yeah, that was my first approach as well - but thought maybe better to keep it both. I changed it.

@vincentbernat
Copy link
Member

Just do if not silent: instead.

Logging to console can be controlled by --silent.

Signed-off-by: Seena Fallah <[email protected]>
@clwluvw clwluvw force-pushed the healthcheck-logging branch from 5339a8a to 49d05e6 Compare March 12, 2024 20:41
@clwluvw
Copy link
Contributor Author

clwluvw commented Mar 12, 2024

Done. PTAL.

@vincentbernat vincentbernat self-requested a review March 12, 2024 20:45
@thomas-mangin thomas-mangin merged commit 3f18086 into Exa-Networks:main Mar 16, 2024
5 checks passed
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.

3 participants