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.

Sign up for GitHub

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

Jump to bottom

Replace logrus for logr+logrus #138

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Replace logrus for logr+logrus #138

wants to merge 5 commits into from

Commits on Apr 29, 2024

  1. actions/interface: Add logger to WipeDisk params 

    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
```
    @mmlb
    mmlb committed Apr 29, 2024
    Configuration menu
    Copy the full SHA
    2105523 View commit details
    Browse the repository at this point in the history

  2. Be consistent with setting/using trace 

    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.
    @mmlb
    mmlb committed Apr 29, 2024
    Configuration menu
    Copy the full SHA
    97bf19a View commit details
    Browse the repository at this point in the history

  3. providers/dell: Replace gotools.assert with stretchr/testify 

    This looks like it came in by mistake since the rest of the code base
uses github.com/stretchr/testify/assert.
    @mmlb
    mmlb committed Apr 29, 2024
    Configuration menu
    Copy the full SHA
    65464c9 View commit details
    Browse the repository at this point in the history

  4. tests: Use test.NewNullLogger from logrus 

    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.
    @mmlb
    mmlb committed Apr 29, 2024
    Configuration menu
    Copy the full SHA
    d835041 View commit details
    Browse the repository at this point in the history

  5. Use logrus via go-logr/logr instead of directly 

    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.
    @mmlb
    mmlb committed Apr 29, 2024
    Configuration menu
    Copy the full SHA
    d663546 View commit details
    Browse the repository at this point in the history