-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[CRITICAL BUG] Build log missing since #1946 #1955
Comments
I believe this is also the root cause for #1954 |
Ugh, that is unfortunate. And entirely my own oversight, since I was too focused on disabling logrus for users who don't want it, and didn't even think about those who would want it. 😞 Let me think on it a bit and see if we can come up with something. It might require changes to ecr-login to make them use a dedicated logger (set to stderr by default), so that it can be selectively disabled without disabling the shared global logger. |
Reported in GoogleContainerTools/kaniko#1955 Changes in awslabs#309 made it possible for callers depending on this code as a Go library to redirect log output to ioutil.Discard to avoid spurious log statements. Unfortunately, because in that change the logger being used was the shared global logger.StandardLogger(), redirecting its output to ioutil.Discard to quiet ecr-login also disabled all logging using the shared global logger (like Kaniko does).
Reported in GoogleContainerTools/kaniko#1955 Changes in awslabs#309 made it possible for callers depending on this code as a Go library to redirect log output to ioutil.Discard to avoid spurious log statements. Unfortunately, because in that change the logger being used was the shared global logger.StandardLogger(), redirecting its output to ioutil.Discard to quiet ecr-login also disabled all logging using the shared global logger (like Kaniko does). Signed-off-by: Jason Hall <jasonhall@redhat.com>
In the meantime, I've patched the ecr-login PR above and built:
...which seems to fix the issue. Please let me know if you see other behavior. |
This is also why integration tests are failing. Tests check for some log output, and don't find it. If the ecr-login change isn't merged soon I may revert the breaking commit, or patch it. Sorry for missing this. |
writes to the requested output , instead of changing the behavior of the standard logrus logger. Rename WithLogOutput to WithLogger to make this new behavior clearer. Use non-global logger to avoid unexpected global log discarding Reported in GoogleContainerTools/kaniko#1955 Changes in awslabs#309 made it possible for callers depending on this code as a Go library to redirect log output to ioutil.Discard to avoid spurious log statements. Unfortunately, because in that change the logger being used was the shared global logger.StandardLogger(), redirecting its output to ioutil.Discard to quiet ecr-login also disabled all logging using the shared global logger (like Kaniko does). Signed-off-by: Jason Hall <jasonhall@redhat.com>
Actual behavior
All logging output from kaniko's log.Info/Warn/Debug are missing
Expected behavior
log.Info should output log
To Reproduce
Steps to reproduce the behavior:
Use docker image built after Bump ecr-login dep to avoid some log spam #1946 for example
gcr.io/kaniko-project/executor:9969c747034aee243a630412c0d74e788eca0d2a
run a basic build with
no-push
everything works fine
there is no log output from kaniko
Additional Information
Reason why this happens
In #1946
https://github.com/GoogleContainerTools/kaniko/pull/1946/files#diff-b0cd856098c47c7aa75c3c9299bb998d62e14e567bced865808d76576a360336R33
Set EcrHelper's logging out put to ioutil.Discard presumably it stops aws from printing things like
no valid credential found
But EcrHelper uses logrus.StandardLogger() which is shared by Kaniko's logger. When setting ecr.log to discard this will actually unexpected set kaniko's log to discard as well.
I didn't make a pull request because im not too sure how we want to fix this
removing ecr junk log is pretty nice but we can't have it remove all kaniko logs 😓
Triage Notes for the Maintainers
--cache
flagThe text was updated successfully, but these errors were encountered: