-
Notifications
You must be signed in to change notification settings - Fork 62
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 more logging to the copr-builder-ready script #3484
Conversation
So it doesn't have an empty stdout when everything went fine: [ INFO][PID:3774651] Checking that builder machine is OK [ INFO][PID:3774651] [ INFO][PID:3774651] Filling build.info file with builder info
Now the logs looks like this for Fedora
And RHEL:
|
@@ -255,6 +255,7 @@ def _check_vm(self): | |||
# generalize it into a separate package that we could eventually use | |||
# here. | |||
cmd = "copr-builder-ready " + self.job.chroot | |||
self.log.info("Running remote command: %s", cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: ton't you want to show stderr, if provided? might give us some warnings in advance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do
if rc:
self.log.info(stderr)
which should IMHO work fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but if rc == 0 and there's some output, we just swallow it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it this way because for some reason my stderr is polluted by this line when testing this locally
Shared connection to builder closed.
So I intentionally wanted to swallow it. But we haven't seen that message in STG so I am confused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷 if you want to hear my opinion, I don't think the logs need to be pretty - but must be complete (minus credentials, haha). But it is up to you, I already approved this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, looks good. Thanks!
So it doesn't have an empty stdout when everything went fine, e.g.