-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Improve progress and history messages for heredoc-related commands #2201
Conversation
lines := strings.Split(strings.TrimSpace(doc), "\n") | ||
first := strings.TrimSpace(lines[0]) | ||
|
||
maxLen := 32 |
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 this code is used to print progress, is it possible to make it "not truncate" when using --progress=plain
? I recall #1435 (also a quick look there, I see it limits at 72 chars, but not sure what's used elsewhere)
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.
^^ discussing with @tonistiigi what options we have. My "case" was that when using --progress=plain
(e.g. in CI, or for debugging purposes), as a user I'd want to see the full output, in case there's an issue somewhere half-way my heredoc.
The debate is if we should do so, because heredocs (quite likely) can be huge if people stuff a lot of code in them. 😓
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 don't necessarily need to trim the first line. Client-side should take care of that(hopefully it doesn't hide command that is more important than heredoc). But I don't think we should start to convert the doc to something else. Yes, you can't see the full contents and I think that is expected, shell does not print out the contents of files you execute either. User can always set set -x
if they want to see internal commands.
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 don't necessarily need to trim the first line.
Yes, perhaps that's an ok balance for now 👍
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 could also make sure that the line is not empty and pick the next one if it is.
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.
My takeaway from this was that we should still show the first (non-empty) line only(in addition to the command). But we should not do any whitespace trimming on that line. Current changes look like the opposite.
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.
Ah ok, my apologies, I misunderstood - will fix that up later.
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.
Have taken a look, now this should just take only the first non-empty line (with no capping) as well as the command.
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.
Where is the non-empty check?
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.
By doing the TrimSpace
above, that would remove all empty lines above - although it would also strip any whitespace at the beginning of the line. Is that something we want to keep as well?
2471e76
to
8bb956c
Compare
8bb956c
to
c99b558
Compare
Signed-off-by: Justin Chadwell <me@jedevc.com>
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 don't know how to test this, but LGTM (from looking at the code). Thanks!
This resolves #2167, following up on providing better progress messages for heredoc commands.
To summarize:
internal
progress messageNot quite sure whether the shortening looks ok, but I kind of like it 😄 You can get messages like:
or