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

Fix --debug flag #878

Merged
merged 2 commits into from
Oct 14, 2024
Merged

Fix --debug flag #878

merged 2 commits into from
Oct 14, 2024

Conversation

wfchandler
Copy link
Contributor

Currently the --debug flag does not actually enable debug logging as
intended. We initialize env_logger immediately in main, then attempt
to change the log filtering level in NewCli::run if the --debug flag
is passed. This does not work, as env_logger::builder needs its init
method to be called to change the global logger. This is not an option,
as we've already called env_logger::init. The internet suggests
that using log::set_max_level lets us change the level independently
of logger initialization, but this did not work when I tried it out.

Move logger initialization into NewCli::run so that we know the
desired log level ahead of calling init. This means that items logged
before we initialize the logger will be lost, but in practice we're not
missing anything.

Currently the `--debug` flag does not actually enable debug logging as
intended. We initialize `env_logger` immediately in `main`, then attempt
to change the log filtering level in `NewCli::run` if the `--debug` flag
is passed. This does not work, as `env_logger::builder` needs its `init`
method to be called to change the global logger. This is not an option,
as we've already called `env_logger::init`. The internet suggests[0]
that using `log::set_max_level` lets us change the level independently
of logger initialization, but this did not work when I tried it out.

Move logger initialization into `NewCli::run` so that we know the
desired log level ahead of calling `init`. This means that items logged
before we initialize the logger will be lost, but in practice we're not
missing anything.

[0] rust-cli/env_logger#228 (comment)
@wfchandler wfchandler requested a review from ahl October 14, 2024 16:45
Comment on lines 580 to 581
.env("OXIDE_HOST", server.url(""))
.env("OXIDE_TOKEN", "oxide-token-bad")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this for auth login?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, good catch. Was trying this test with auth login and auth status and forgot to remove them.

Fixed in 3c306c0

@ahl ahl merged commit 067caa6 into main Oct 14, 2024
16 checks passed
@ahl ahl deleted the wc/fix-debug branch October 14, 2024 18:32
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.

2 participants