-
-
Notifications
You must be signed in to change notification settings - Fork 636
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
docker: log image pulls #16849
docker: log image pulls #16849
Conversation
status: Some(status), | ||
.. | ||
} => { | ||
log::info!("Docker pull status: {status}"); |
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.
Maybe this is better as debug
?
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.
These messages are what the user would see when running docker pull
manually. I'd like to provide the same UX for the moment. If it's too chatty, we can always put it to debug level.
} | ||
|
||
log::info!("Finished pull of Docker image `{image}`."); |
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.
Consider using a workunit, rather than a pair of log messages?
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.
Good idea. How should the status message be logged then? debug level?
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.
Also the actual pull_image
function is debounced via a OnceCell
. Will that affect associating the work unit with the correct parent?
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.
Good idea. How should the status message be logged then? debug level?
The workunit's level (and whether the UI is in use) drives how it is logged. When the UI is enabled, DEBUG or greater are rendered in the UI, but only INFO or greater have their completion logged. When the UI is disabled, debug won't render at all (unless it is a "Long running workunit..."), but INFO will render both start and end.
Also the actual
pull_image
function is debounced via aOnceCell
. Will that affect associating the work unit with the correct parent?
It won't, no.
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.
Should the in_workunit! call be inside the OnceCell get_or_try_init or outside of 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.
Inside. Otherwise the workunit will be created even if nothing needs to be done.
[ci skip-build-wheels]
[ci skip-build-wheels]
c8d0841
to
73c1137
Compare
Log Docker image pulls, otherwise Pants will appear to hang if Pants decides it is necessary to pull an image.
[ci skip-build-wheels]