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

fix(log): Send Go std log to logrus, and output ggcr logs #6815

Merged
merged 2 commits into from
Nov 11, 2021
Merged

fix(log): Send Go std log to logrus, and output ggcr logs #6815

merged 2 commits into from
Nov 11, 2021

Conversation

halvards
Copy link
Contributor

@halvards halvards commented Nov 9, 2021 •
edited
Loading

This change directs log messages sent to Go's standard log package to logrus instead.

Also, previously, Skaffold discarded log messages from go-containerregistry (a.k.a. ggcr).
This change enables ggcr log messages, with severity tied to Skaffold's log level.

Related: #6041

@halvards halvards requested a review from a team as a code owner November 9, 2021 04:22
@google-cla google-cla bot added the cla: yes label Nov 9, 2021
briandealwis
briandealwis reviewed Nov 9, 2021
View reviewed changes
Copy link
Member

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

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

LGTM. @MarlonGamez any concerns?

@codecov
Copy link

codecov bot commented Nov 9, 2021 •
edited
Loading

Codecov Report

Merging #6815 (f5e50ad) into main (290280e) will decrease coverage by 1.32%.
The diff coverage is 64.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6815      +/-   ##
==========================================
- Coverage   70.48%   69.16%   -1.33%     
==========================================
  Files         515      547      +32     
  Lines       23150    25049    +1899     
==========================================
+ Hits        16317    17324    +1007     
- Misses       5776     6564     +788     
- Partials     1057     1161     +104
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/deploy.go 52.00% <ø> (-1.85%) ⬇️
cmd/skaffold/app/cmd/dev.go 84.61% <0.00%> (ø)
cmd/skaffold/app/cmd/flags.go 91.00% <0.00%> (+0.18%) ⬆️
cmd/skaffold/app/cmd/render.go 36.66% <0.00%> (-4.72%) ⬇️
cmd/skaffold/skaffold.go 0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/inspect_tests.go 62.50% <14.28%> (-1.14%) ⬇️
cmd/skaffold/app/cmd/lint.go 42.85% <42.85%> (ø)
cmd/skaffold/app/cmd/find_configs.go 48.88% <50.00%> (+0.24%) ⬆️
cmd/skaffold/app/skaffold.go 76.19% <70.00%> (-8.43%) ⬇️
cmd/skaffold/app/cmd/inspect_build_env.go 65.11% <75.00%> (+6.39%) ⬆️
... and 161 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6cfc939...f5e50ad. Read the comment docs.

@halvards
Copy link
Contributor Author

halvards commented Nov 9, 2021

The unit test failure is puzzling, I haven't been able to reproduce it locally.

@halvards
Copy link
Contributor Author

halvards commented Nov 9, 2021

I have an idea for how to improve this to cover other libraries that log to Go's standard log package (e.g., ko), so I'll update this PR to cover that.

Marking as Draft in the meantime.

@halvards halvards marked this pull request as draft November 9, 2021 06:25
@pull-request-size pull-request-size bot added size/L and removed size/M labels Nov 9, 2021
@halvards halvards changed the title fix(log): Output ggcr logs fix(log): Send Go std log to logrus, and output ggcr logs Nov 9, 2021
@halvards halvards added the kokoro:run runs the kokoro jobs on a PR label Nov 9, 2021
@halvards halvards marked this pull request as ready for review November 9, 2021 11:16
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Nov 9, 2021
@halvards halvards marked this pull request as draft November 9, 2021 11:25
MarlonGamez
MarlonGamez reviewed Nov 9, 2021
View reviewed changes
Copy link
Contributor

@MarlonGamez MarlonGamez left a comment

Choose a reason for hiding this comment

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

Sorry, I know this is draft but just had a QQ

pkg/skaffold/output/log/log.go Show resolved Hide resolved
@halvards halvards force-pushed the ggcr-logs branch 2 times, most recently from 9594d7e to 6bb239c Compare November 10, 2021 06:10
@halvards halvards marked this pull request as ready for review November 10, 2021 06:38
@halvards halvards added the kokoro:run runs the kokoro jobs on a PR label Nov 10, 2021
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Nov 10, 2021
@halvards
Copy link
Contributor Author

Ok, this one is (finally) ready.

@tejal29
Copy link
Member

tejal29 commented Nov 11, 2021

I just want to make sure, this works well with Cloud code output logging. Lets try to test this PR from cloud code side in tomorrow's bug bash.

@halvards
fix(log): Output ggcr logs
955a754 
Previously, Skaffold discarded log messages from
[`go-containerregistry`](https://github.com/google/go-containerregistry)
(a.k.a `ggcr`).

This change enables `ggcr` log messages, with severity tied to
Skaffold's log level.

Related: GoogleContainerTools#6041
@halvards
fix(log): Send Go std log to logrus
f5e50ad 
Includes `ggcr` logs.
@halvards halvards added the kokoro:run runs the kokoro jobs on a PR label Nov 11, 2021
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Nov 11, 2021
@halvards
Copy link
Contributor Author

Just rebased the commits.

@tejal29 tejal29 enabled auto-merge (squash) November 11, 2021 19:46
@tejal29 tejal29 merged commit 2330430 into GoogleContainerTools:main Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarlonGamez MarlonGamez MarlonGamez left review comments

@briandealwis briandealwis briandealwis left review comments

@tejal29 tejal29 tejal29 approved these changes

Assignees

@MarlonGamez MarlonGamez

Labels
cla: yes size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@halvards @tejal29 @briandealwis @MarlonGamez @kokoro-team