Skip to content

refactor to use util/bklog.G(ctx) instead logrus. directly#2236

Merged
tonistiigi merged 1 commit intomoby:masterfrom
morlay:master
Jul 13, 2021
Merged

refactor to use util/bklog.G(ctx) instead logrus. directly#2236
tonistiigi merged 1 commit intomoby:masterfrom
morlay:master

Conversation

@morlay
Copy link
Copy Markdown
Contributor

@morlay morlay commented Jul 9, 2021

Too may changes

I'm not sure, Should I add new param ctx context.Context, specially for public functions

@morlay morlay force-pushed the master branch 3 times, most recently from 200441f to a278098 Compare July 9, 2021 07:48
Copy link
Copy Markdown
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Generally looks ok to me. Do I understand correctly that bklog.G(ctx) can be stored as a value and use multiple times to generate a log?

Comment thread util/bklog/log.go
Comment thread control/control.go Outdated
@morlay
Copy link
Copy Markdown
Contributor Author

morlay commented Jul 13, 2021

bklog.G(ctx) can be stored as a value and use multiple times to generate a log?

Yes, but could only as variable in a function scope

@morlay morlay marked this pull request as ready for review July 13, 2021 02:52
@morlay
Copy link
Copy Markdown
Contributor Author

morlay commented Jul 13, 2021

@tonistiigi

  • removed all logrus. in each pkg (exclude test files).
  • added context.Context as first parameter if required.
  • use bklog.G(context.TODO()) for hard to refactor, like solver, may next PR to do more refactor.

Comment thread client/solve.go Outdated
Comment thread worker/base/worker.go Outdated
@tonistiigi tonistiigi requested a review from AkihiroSuda July 13, 2021 03:01
@morlay morlay force-pushed the master branch 6 times, most recently from ee22977 to b715f9a Compare July 13, 2021 03:38
Signed-off-by: Morlay <morlay.null@gmail.com>
@tonistiigi tonistiigi merged commit 06e8602 into moby:master Jul 13, 2021
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