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

Replace println! from library functions with configurable logging #424

Merged
merged 4 commits into from
Apr 10, 2024

Conversation

mishazharov
Copy link
Contributor

@mishazharov mishazharov commented Apr 1, 2024

Developers building on this crate do not necessarily need logging to occur over println! when authenticating 2FA, so this commit replaces the print macros with logging macros

Fixes #

  • cargo test has been run and passes
  • documentation has been updated with relevant examples (if relevant)

Developers building on this crate do not necessarily need logging to
occur over `println!` when authenticating 2FA, so this commit replaces
the print macros with logging macros
webauthn-authenticator-rs/src/mozilla.rs Outdated Show resolved Hide resolved
@mishazharov mishazharov requested a review from yaleman April 3, 2024 04:21
@mishazharov
Copy link
Contributor Author

It looks like there's some CI failures unrelated to this change:

error: package `bumpalo v3.15.4` cannot be built because it requires rustc 1.73.0 or newer, while the currently active rustc version is 1.70.0
Either upgrade to rustc 1.73.0 or newer, or use
cargo update -p bumpalo@3.15.4 --precise ver
where `ver` is the latest version of `bumpalo` supporting rustc 1.70.0
Error: Process completed with exit code 101.

@yaleman
Copy link
Member

yaleman commented Apr 3, 2024

@Firstyear how's this one look? Figured I'd ask because you've used it more

yaleman
yaleman previously approved these changes Apr 3, 2024
Copy link
Member

@Firstyear Firstyear left a comment

Choose a reason for hiding this comment

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

Actually @micolous would be the person to ask. There are some places where I think we use these messages as communication points for cli tools.

webauthn-authenticator-rs/src/mozilla.rs Outdated Show resolved Hide resolved
@micolous
Copy link
Collaborator

micolous commented Apr 8, 2024

Actually @micolous would be the person to ask. There are some places where I think we use these messages as communication points for cli tools.

All the other backends use info!, error!, etc.; the Mozilla backend is the only one which doesn't.

The CLI tools I wrote touch the other backends (as they need to interact with it on a CTAP2 level, rather than an "Authenticator" level), so this would at least make things consistent (as they will eventually replace the Mozilla backend).

I believe the main user of the Mozilla backend is Kanidm's CLI tools – I don't know what makes sense there.

@micolous micolous requested a review from Firstyear April 8, 2024 07:00
@yaleman
Copy link
Member

yaleman commented Apr 8, 2024

Libraries really should use the log primitives instead of println so the dependent apps can do what they need to from there...

@yaleman yaleman merged commit 869a0b9 into kanidm:master Apr 10, 2024
33 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.

4 participants