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

Print logs from init containers #2182

Merged

Conversation

corneliusweig
Copy link
Contributor

This is based on #1865, but now skips terminated containers.

Close #1865.
CC @tejal29 who raised this concern on #1865 and @ottonello who opened #1865 .

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@corneliusweig
Copy link
Contributor Author

@ottonello Looks like you need to explicitly express your consent that my additional changes are ok (https://opensource.google.com/docs/cla/#github-consent).

@codecov-io
Copy link

Codecov Report

Merging #2182 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2182      +/-   ##
==========================================
- Coverage   57.41%   57.39%   -0.03%     
==========================================
  Files         187      187              
  Lines        7905     7908       +3     
==========================================
  Hits         4539     4539              
- Misses       2954     2957       +3     
  Partials      412      412
Impacted Files Coverage Δ
pkg/skaffold/kubernetes/log.go 20.8% <0%> (-0.52%) ⬇️

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 d458fa5...b25582d. Read the comment docs.

Copy link
Contributor

@ottonello ottonello left a comment

Choose a reason for hiding this comment

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

Thanks for this! Should this address the concerns in #1865 ? I got stuck at trying to write some integration tests, none of them were really ready for testing something like this.

@corneliusweig
Copy link
Contributor Author

@ottonello I totally missed that there were other issues with the original PR.
I don't think that the kaniko failure is related to this change though, at least I could not reproduce an error.

I'll try to set up an integration test for logs from init containers.

@tejal29 tejal29 added cla: yes and removed cla: no labels May 28, 2019
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@tejal29 tejal29 added the kokoro:run runs the kokoro jobs on a PR label May 28, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label May 28, 2019
@tejal29
Copy link
Contributor

tejal29 commented May 28, 2019

Thanks @corneliusweig for taking this up again. Running kokoro now.
Unfortunately there are no tests for log.go :(. Maybe we can cover it later

@tejal29 tejal29 merged commit 1abf045 into GoogleContainerTools:master May 28, 2019
@corneliusweig corneliusweig deleted the pr/log-init-containers branch May 29, 2019 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants