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.

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

Add newlines to SkaffoldLogEvents that come from logrus #6506

Merged
merged 1 commit into from
Aug 24, 2021

Conversation

MarlonGamez
Copy link
Contributor

@MarlonGamez MarlonGamez commented Aug 24, 2021

Related: #6492

Description
This PR adds line breaks to the end of messages that come from logrus and get sent out through SkaffoldLogEvents.
Before, we sent entry.Message, but now we will send fmt.Sprintf("%s\n", entry.Message)

Also fixes a few log lines that should be the ln form of their function call.

Follow-up Work (remove if N/A)
Another PR will be opened to add a default log level for basic output that comes from skaffold. That PR will close #6492

@MarlonGamez MarlonGamez requested a review from a team as a code owner August 24, 2021 20:44
@google-cla google-cla bot added the cla: yes label Aug 24, 2021
@@ -138,7 +138,7 @@ func (c *cache) Build(ctx context.Context, out io.Writer, tags tag.ImageTags, ar
})
}

log.Entry(ctx).Info("Cache check completed in", util.ShowHumanizeTime(time.Since(start)))
log.Entry(ctx).Infoln("Cache check completed in", util.ShowHumanizeTime(time.Since(start)))
Copy link
Contributor

@aaron-prindle aaron-prindle Aug 24, 2021

Choose a reason for hiding this comment

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

nit: Are there any integration/unit tests that verify the form of a log entries? Might be helpful if Cloud Code relies on them

Copy link
Contributor Author

@MarlonGamez MarlonGamez Aug 24, 2021

Choose a reason for hiding this comment

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

We currently don't really have any testing for most things output related I believe. This would be good to have in the future and I think that having the API Server now will help a lot with that since we can connect to the server and check output through there rather than parsing output from the terminal. I'd like to add this is a follow up

@codecov
Copy link

codecov bot commented Aug 24, 2021

Codecov Report

Merging #6506 (8c05e31) into main (c75c551) will decrease coverage by 0.01%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6506      +/-   ##
==========================================
- Coverage   70.42%   70.41%   -0.02%     
==========================================
  Files         515      515              
  Lines       23121    23121              
==========================================
- Hits        16284    16281       -3     
- Misses       5781     5782       +1     
- Partials     1056     1058       +2     
Impacted Files Coverage Δ
pkg/skaffold/event/v2/logger.go 76.27% <0.00%> (ø)
pkg/skaffold/build/cache/retrieve.go 64.40% <100.00%> (ø)
pkg/skaffold/runner/build.go 74.28% <100.00%> (ø)
pkg/skaffold/runner/v1/dev.go 73.12% <100.00%> (ø)
pkg/skaffold/docker/image.go 65.56% <0.00%> (-1.00%) ⬇️

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 c75c551...8c05e31. Read the comment docs.

@MarlonGamez MarlonGamez merged commit da9ffe3 into GoogleContainerTools:main Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent line breaks in LogEvent messages (V2 API)
2 participants