Skip to content

tuf: move INFO logs to DEBUG or WARNING #2243

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

Merged
merged 4 commits into from
Dec 27, 2022

Conversation

woodruffw
Copy link
Contributor

This should reduce some default logging noise when TUF's ngclient is used as a library in other applications (in particular, sigstore-python).

See: sigstore/sigstore-python#351

cc @jku

Signed-off-by: William Woodruff william@trailofbits.com

Signed-off-by: William Woodruff <william@trailofbits.com>
@coveralls
Copy link

coveralls commented Dec 20, 2022

Pull Request Test Coverage Report for Build 3758797915

  • 8 of 8 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 98.194%

Totals Coverage Status
Change from base Build 3731403975: 0.0%
Covered Lines: 1349
Relevant Lines: 1365

💛 - Coveralls

Signed-off-by: William Woodruff <william@trailofbits.com>
Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

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

This should reduce some default logging noise when TUF's ngclient is used as a library in other applications (in particular, sigstore-python).

Thanks. I think I agree with this... but I will note that
a) sigstore-python is the one setting a non-default log level (WARNING is default IIRC)
b) maybe there is documentation on what should be considered INFO and what DEBUG but I haven't found any. I think your interpretation is that INFO is something that can be exposed to end users and DEBUG is not: that sounds reasonable but it also means libraries then essentially have one log level available to them 🤷

That one warning should be debug, otherwise LGTM.

@woodruffw
Copy link
Contributor Author

a) sigstore-python is the one setting a non-default log level (WARNING is default IIRC)

Oh, you're completely right. I'm not sure why we set INFO as our default level; that might be an artifact of me originally intending to use the logger for more regular outputs.

Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw requested a review from jku December 21, 2022 15:34
Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

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

I think you modified the wrong warning

Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw requested a review from jku December 22, 2022 14:57
Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

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

looks good to me. I'm not 100% sure what the correct way to deal with logger.info() is (as this approach essentially means that libraries are only allowed to log at debug, and info essentially does not exist for libraries: this feels wrong) but I agree this change is in line with other logger calls.

We likely debug-log a little too much at the moment in ngclient but these are all useful logs so that's another issue.

@jku jku merged commit 41a2035 into theupdateframework:develop Dec 27, 2022
@woodruffw woodruffw deleted the ww/recategorize-logs branch December 27, 2022 16:41
@woodruffw
Copy link
Contributor Author

Yeah, agreed -- I need to re-think how sigstore-python does loglevels more generally (it probably makes sense for INFO to be the default for first-party code, DEBUG for third-party, etc.)

I'm going to file an issue for that.

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