feat(logging): using new package containing logrus.Logger instead of global scope logrus (closes #583) (#699, @Shanduur) #699

shanduur · 2021-08-02T06:36:04Z

What

This PR is intended to close issue #583 . New package logger was introduced and all usages of logrus.Something were replaced with l.Log().Something, e.g.:

-import log "github.com/sirupsen/logrus"
+import l "github.com/rancher/k3d/v4/logger"

/* ... */
-	log.Errorf(/* ... */
+	l.Log().Errorf(/* ... */

Why

Due to the #583 .

Implications

It does not affect the behavior of the CLI, but it can affect all 3rd party users of k3d packages. As the global scope logrus is no longer used, it can create mismatch between log level of k3d packages and other packages importing k3d packages using global scope logrus.

iwilltry42 · 2021-08-16T12:45:04Z

Hi @shanduur , thanks for opening this PR!
That's a great change, that will also solve some issues I faced recently, where the docker dependency overwrote log settings of k3d, as both used the global context logrus instance 🙄
However, this will go into v5.0.0... any chance you could re-do this from main-v5 branch? 🤔

EDIT: I changed the PR base branch to main-v5 already and obviously we run into some merge conflicts now. Also I guess, basing it off of this new branch, some changes will be missing from new files 🤔

EDIT 2: if that's too much work and you don't have the time to do it, I can try my hand on porting your changes accordingly. Let me know 👍

shanduur · 2021-08-16T12:58:55Z

I'll fix that!

Signed-off-by: Mateusz Urbanek <mateusz.urbanek.98@gmail.com>

shanduur · 2021-08-17T05:19:02Z

@iwilltry42 could you look at failed tests? I am not sure if this is something broken by me.

iwilltry42

I have to admit.. that was a quite repetitive review 😂
Thanks a lot for your effort in this @shanduur !

I have some change requests though, which are rather small and most result from the search-replace flow I guess.

UPDATE: committed the suggestions now, so the PR should be ready to merge once the tests pass ✔️

docs/usage/guides/calico.yaml

docs/static/js/asciinema-player.js

iwilltry42 · 2021-08-18T07:55:39Z

cmd/root.go

 			} else if pluginFound {
 				os.Exit(0)
 			}
 		}
 	}
 	if err := cmd.Execute(); err != nil {
-		log.Fatalln(err)
+		l.Log().Fatalln(err)
 	}
 }

 // initLogging initializes the logger
 func initLogging() {


TODO for later (no need to do it in this PR): move this to the logger package with functions to set k3d defaults and proper config structs.
I guess this would make sense to have it defined in pkg/ instead of the CLI (cmd/) path/package.

iwilltry42 · 2021-08-18T08:04:45Z

pkg/runtimes/docker/util.go

@@ -175,7 +175,7 @@ func GetDockerClient() (*client.Client, error) {
 	}

 	newClientOpts := flags.NewClientOptions()
-	newClientOpts.Common.LogLevel = log.GetLevel().String() // this is needed, as the following Initialize() call will set a new log level on the global logrus instance
+	newClientOpts.Common.LogLevel = l.Log().GetLevel().String() // this is needed, as the following Initialize() call will set a new log level on the global logrus instance


I guess this could be dropped now, as k3d itself doesn't use the global logrus instance anymore.. but it's not that important and maybe it could even be useful to cascade the log level down to the docker client 🤔

tools/cmd/image.go

tools/main.go

Co-authored-by: Thorsten Klein <iwilltry42@gmail.com>

iwilltry42 · 2021-08-18T11:27:39Z

Thanks a lot for the time and effort @shanduur :)

shanduur · 2021-08-18T11:38:03Z

@all-contributors please add @shanduur for code

allcontributors · 2021-08-18T11:38:11Z

@shanduur

I've put up a pull request to add @shanduur! 🎉

shanduur changed the title ~~[Enhancement] using new package containing logrus.Logger instead of global scope logrus (closes #583)~~ feat(logging): using new package containing logrus.Logger instead of global scope logrus (closes #583) Aug 2, 2021

iwilltry42 added this to the v5.0.0 milestone Aug 12, 2021

iwilltry42 added the breaking change label Aug 12, 2021

iwilltry42 self-requested a review August 16, 2021 12:42

iwilltry42 assigned iwilltry42 and shanduur Aug 16, 2021

iwilltry42 changed the base branch from main to main-v5 August 16, 2021 12:45

shanduur force-pushed the feature/logger branch from 53675c2 to b8f7587 Compare August 16, 2021 14:32

feat: replaced global scope logrus with instantiated logrus.Logger

5e073f8

Signed-off-by: Mateusz Urbanek <mateusz.urbanek.98@gmail.com>

shanduur force-pushed the feature/logger branch from b8f7587 to 5e073f8 Compare August 16, 2021 14:44

iwilltry42 requested changes Aug 18, 2021

View reviewed changes

shanduur and others added 5 commits August 18, 2021 10:31

Update docs/usage/guides/calico.yaml

4f8b2c5

Co-authored-by: Thorsten Klein <iwilltry42@gmail.com>

Update tools/main.go

3de66b8

Update tools/cmd/image.go

624491f

Update docs/static/js/asciinema-player.js

624101d

Merge branch 'main-v5' into feature/logger

4cb75e6

iwilltry42 self-requested a review August 18, 2021 10:19

iwilltry42 approved these changes Aug 18, 2021

View reviewed changes

iwilltry42 changed the title ~~feat(logging): using new package containing logrus.Logger instead of global scope logrus (closes #583)~~ feat(logging): using new package containing logrus.Logger instead of global scope logrus (closes #583) (#699, @Shanduur) Aug 18, 2021

iwilltry42 merged commit 917c19e into k3d-io:main-v5 Aug 18, 2021

allcontributors bot mentioned this pull request Aug 18, 2021

docs: add Shanduur as a contributor for code #713

Merged

iwilltry42 mentioned this pull request Aug 18, 2021

[FEATURE] Plugin System #714

Closed

12 tasks

iwilltry42 linked an issue Aug 18, 2021 that may be closed by this pull request

[Enhancement] Better to use logrus.New to replace the global scope logrus #583

Closed

FedericoAntoniazzi added a commit to FedericoAntoniazzi/k3d that referenced this pull request Aug 18, 2021

[FIX] log: Adjust logger according to k3d-io#699

574723c

FedericoAntoniazzi added a commit to FedericoAntoniazzi/k3d that referenced this pull request Aug 31, 2021

[FIX] log: Adjust logger according to k3d-io#699

d4494c2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(logging): using new package containing logrus.Logger instead of global scope logrus (closes #583) (#699, @Shanduur) #699

feat(logging): using new package containing logrus.Logger instead of global scope logrus (closes #583) (#699, @Shanduur) #699

shanduur commented Aug 2, 2021 •

edited

Loading

iwilltry42 commented Aug 16, 2021 •

edited

Loading

shanduur commented Aug 16, 2021

shanduur commented Aug 17, 2021

iwilltry42 left a comment •

edited

Loading

iwilltry42 Aug 18, 2021

iwilltry42 Aug 18, 2021

iwilltry42 commented Aug 18, 2021

shanduur commented Aug 18, 2021

allcontributors bot commented Aug 18, 2021

feat(logging): using new package containing logrus.Logger instead of global scope logrus (closes #583) (#699, @Shanduur) #699

feat(logging): using new package containing logrus.Logger instead of global scope logrus (closes #583) (#699, @Shanduur) #699

Conversation

shanduur commented Aug 2, 2021 • edited Loading

What

Why

Implications

iwilltry42 commented Aug 16, 2021 • edited Loading

shanduur commented Aug 16, 2021

shanduur commented Aug 17, 2021

iwilltry42 left a comment • edited Loading

Choose a reason for hiding this comment

iwilltry42 Aug 18, 2021

Choose a reason for hiding this comment

iwilltry42 Aug 18, 2021

Choose a reason for hiding this comment

iwilltry42 commented Aug 18, 2021

shanduur commented Aug 18, 2021

allcontributors bot commented Aug 18, 2021

shanduur commented Aug 2, 2021 •

edited

Loading

iwilltry42 commented Aug 16, 2021 •

edited

Loading

iwilltry42 left a comment •

edited

Loading