Replace logrus for logr+logrus #138
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We want to be able to log/print progress in WipeDisk since disk wiping may take a long time. Instead of using the global logger from log package and assuming thats the right thing to do we should take it in as a parameter. This also gets rid of log messages printed during tests while running either from within utils directory or by running with -v: Before: ``` go test 2024/04/26 14:47:34 Starting zero-fill of /run/user/1000/tmp/example1832453771 2024/04/26 14:47:34 /run/user/1000/tmp/example1832453771 | Size: 4096B 2024/04/26 14:47:34 /run/user/1000/tmp/example1832453771 | Progress: 100.00% | Speed: 1676.50 MB/s | Estimated time left: 0.00 hour(s) 2024/04/26 14:47:34 Starting zero-fill of /run/user/1000/tmp/example1376210307 2024/04/26 14:47:34 /run/user/1000/tmp/example1376210307 | Size: 4096B 2024/04/26 14:47:34 /run/user/1000/tmp/example1376210307 | Progress: 100.00% | Speed: 3030.45 MB/s | Estimated time left: 0.00 hour(s) 2024/04/26 14:47:34 Starting zero-fill of /run/user/1000/tmp/example3643202665 2024/04/26 14:47:34 /run/user/1000/tmp/example3643202665 | Size: 4096B 2024/04/26 14:47:34 /run/user/1000/tmp/example3643202665 | Progress: 100.00% | Speed: 3004.81 MB/s | Estimated time left: 0.00 hour(s) 2024/04/26 14:47:34 Starting zero-fill of /run/user/1000/tmp/example2153188305 2024/04/26 14:47:34 /run/user/1000/tmp/example2153188305 | Size: 4096B 2024/04/26 14:47:34 /run/user/1000/tmp/example2153188305 | Progress: 100.00% | Speed: 2981.87 MB/s | Estimated time left: 0.00 hour(s) PASS ok github.com/metal-toolbox/ironlib/utils 0.100s ``` After: ``` go test PASS ok github.com/metal-toolbox/ironlib/utils 0.100s ```
Not a fan of how this ended up but at least its all consistent. Before this change the code was all over the place with how it decided to check if Trace was enabled or not (== vs >=). Also most structs had a trace field that was never set and thus not needed but I decided to use the field instead of removing it since it is being used by the firmware checksum collector and I'd rather they all do it the same way.
This looks like it came in by mistake since the rest of the code base uses github.com/stretchr/testify/assert.
This way we avoid log messages to the console during tests when running from within same directory as tests. Looking at providers/dell we see: Before: ``` go test INFO[0000] Collecting hardware inventory INFO[0000] nil firmware checksum collector INFO[0000] update parameters base url= dsu repo= dsu version= INFO[0000] Dell DSU prerequisites setup complete INFO[0000] Identifying component firmware updates... INFO[0000] update parameters base url= dsu repo= dsu version= INFO[0000] Dell DSU prerequisites setup complete INFO[0000] component updates identified.. count=5 PASS ok github.com/metal-toolbox/ironlib/providers/dell 0.007s ``` After: ``` go test PASS ok github.com/metal-toolbox/ironlib/providers/dell 0.006s ``` And similar for the other tests that were changed.
logr gives us a few benefits over logrus. 1. Its a generic interface for multiple backends vs logrus being a direct implementation. This allows us to swap out the backends as needed (will be useful soon). 2. Being backend agnostic we can use a backend that supports testing.T.Log which avoids logging output when its unneeded. Another slight benefit is that logrus has gone into maintenance mode but is not really "done". I'm considering lack of testing.T integration an importang missing feature that may never get implemented. The most common hack I'v seen is to just write logs during test to io.Discard, but this is unsatisfactory.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
logrus is in maintenance mode and already missing some nice features like better integration with testing.T.Log methods that most other log libraries have. So I took a stab at replacing logrus with logr + logrus so we can check it out. If we go this route we could probably replace logrus with zap or some other backend later pretty easily.