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

Don't unmute logs if an error happened #928

Merged
merged 2 commits into from
Sep 7, 2018

Conversation

dgageot
Copy link
Contributor

@dgageot dgageot commented Aug 23, 2018

I'm not fully happy with the code because it introduces some duplication
But that could be dealt with in another PR

Fix #923

Signed-off-by: David Gageot david@gageot.net

@dgageot dgageot added the wip label Aug 23, 2018
@dgageot dgageot requested review from balopat and r2d4 as code owners August 23, 2018 16:12
@codecov-io
Copy link

codecov-io commented Aug 23, 2018

Codecov Report

Merging #928 into master will decrease coverage by 0.15%.
The diff coverage is 55.88%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #928      +/-   ##
=========================================
- Coverage   43.55%   43.4%   -0.16%     
=========================================
  Files          63      63              
  Lines        2645    2652       +7     
=========================================
- Hits         1152    1151       -1     
- Misses       1373    1379       +6     
- Partials      120     122       +2
Impacted Files Coverage Δ
pkg/skaffold/kubernetes/log.go 3.96% <0%> (-0.14%) ⬇️
pkg/skaffold/runner/runner.go 54.78% <67.85%> (-1.43%) ⬇️

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 0592b2b...ade6898. Read the comment docs.

Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

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

I would love to see a test case or two around this + see the nit comment.

@@ -207,23 +207,41 @@ func (r *SkaffoldRunner) Dev(ctx context.Context, out io.Writer, artifacts []*v1
changed := changes{}
onChange := func() error {
logger.Mute()
unmute := true
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rename this to hasError, I think that would be more readable, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

ping @dgageot ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, hasError is much better! I though I pushed this fix but I must have lost it.

@dgageot dgageot force-pushed the mute-errors branch 2 times, most recently from 73bfeb7 to 3033e69 Compare September 5, 2018 14:54
Fix GoogleContainerTools#923

Signed-off-by: David Gageot <david@gageot.net>
When the skaffold configuration is changed, we
Forgot to stop the logger before starting a new
one.


Signed-off-by: David Gageot <david@gageot.net>
@dgageot
Copy link
Contributor Author

dgageot commented Sep 6, 2018

Rebasing and making sure the kokoro tests pass

@dgageot dgageot merged commit ef2af2d into GoogleContainerTools:master Sep 7, 2018
@dgageot dgageot deleted the mute-errors branch December 28, 2018 07:11
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