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

Wrap tester in SkaffoldRunner to improve SkaffoldLogEvent labelling #6469

Merged
merged 2 commits into from Aug 24, 2021

Conversation

MarlonGamez
Copy link
Contributor

@MarlonGamez MarlonGamez commented Aug 20, 2021

Fixes: #5368

Description
This PR adds proper labelling of SkaffoldLogEvents that come from Skaffold's testing phase. I've also added a Test function onto the SkaffoldRunner rather than embedding a tester. This allows us to use output.WithEventContext in a way that's more consistent with how we use it for build and deploy phases.

SkaffoldLogEvents starting from Starting Test... up to, but not including, Tags used in deployment should now have proper task information set.

Tests were not added for SkaffoldRunner.Test as there is no real logic there, it's simply calling the underlying data structures Test function.

@MarlonGamez MarlonGamez requested a review from a team as a code owner August 20, 2021 00:28
@google-cla google-cla bot added the cla: yes label Aug 20, 2021
@codecov
Copy link

codecov bot commented Aug 20, 2021

Codecov Report

Merging #6469 (afbaf6d) into main (8303f00) will decrease coverage by 0.01%.
The diff coverage is 90.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6469      +/-   ##
==========================================
- Coverage   70.28%   70.26%   -0.02%     
==========================================
  Files         510      511       +1     
  Lines       22988    22993       +5     
==========================================
- Hits        16157    16156       -1     
- Misses       5775     5779       +4     
- Partials     1056     1058       +2     
Impacted Files Coverage Δ
pkg/skaffold/runner/v1/dev.go 73.12% <0.00%> (ø)
pkg/skaffold/runner/v1/runner.go 0.00% <ø> (ø)
pkg/skaffold/test/test_factory.go 76.19% <ø> (-1.09%) ⬇️
pkg/skaffold/runner/v1/new.go 63.24% <100.00%> (ø)
pkg/skaffold/runner/v1/test.go 100.00% <100.00%> (ø)
pkg/skaffold/log/stream/stream.go 71.42% <0.00%> (-14.29%) ⬇️
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 8303f00...afbaf6d. Read the comment docs.

@tejal29
Copy link
Member

tejal29 commented Aug 20, 2021

The code changes look good. Can you please add the new event changes?

Copy link
Member

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

Please specify the event difference.

@MarlonGamez
Copy link
Contributor Author

@tejal29 SkaffoldLogEvents starting from Starting Test... up to, but not including, Tags used in deployment should now have proper task information set. I updated the PR description to include this information 👍🏼

@tejal29 tejal29 merged commit 7760f45 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.

Implement API v2
2 participants