-
Notifications
You must be signed in to change notification settings - Fork 177
Conversation
You have different IDs as we now insert the build time into the |
@silvin-lubecki Yep! And I think that's strange/inconsistent. |
b9308ef
to
89a9b24
Compare
89a9b24
to
0c3e2e6
Compare
Codecov Report
@@ Coverage Diff @@
## master #771 +/- ##
======================================
Coverage 71.1% 71.1%
======================================
Files 67 67
Lines 3835 3835
======================================
Hits 2727 2727
Misses 756 756
Partials 352 352 Continue to review full report at Codecov.
|
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.
LGTM
0c3e2e6
to
4c5ee74
Compare
internal/commands/build/build.go
Outdated
return nil, err | ||
} | ||
once.Do(func() { | ||
fmt.Fprintf(out, "Successfully built app %s\n", id.String()) |
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.
The text changed, is that ok?
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.
This is something I would like to discuss.
The SHA that this message shows is not an image, but a bundle. This is exactly the same message shown by the "normal builder" and docker-compose
to show the SHA of a docker image
.
IMO, that's confusing to have this message building another thing and saying the same.
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.
Well, actually it's an image... Not a container image but still :)
Any opinion @ndeloof as you first wrote this log I guess?
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.
"app image" is the recommended name for this (se naming doc)
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.
Reason to show status this way is to "mimic the docker CLI" as our UX mantra :P
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.
hmmm... Looks like now we have 3 alternatives:
- "Successfully built"
- "Successfully built app"
- "Successfully built app image"
What do you think @chris-crone @zappy-shu @aiordache ?
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.
With BuildKit the final line is:
=> => writing image sha256:6f71466d421e3ab378cf91c03bfd4d00ecfc1ba7dc3a37ccb69c02b2ef0a2f6b
Without, it's:
Successfully built 965ea09ff2eb
I would say that we should use option 3 to make sure that it's clear.
4c5ee74
to
7db0f3a
Compare
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 think we are also missing a test here 😸
internal/commands/build/build.go
Outdated
return nil, err | ||
} | ||
once.Do(func() { | ||
fmt.Fprintf(out, "Successfully built app %s\n", id.String()) |
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.
Reason to show status this way is to "mimic the docker CLI" as our UX mantra :P
0e8240e
to
5e5ee8f
Compare
5e5ee8f
to
f8468a8
Compare
Signed-off-by: Ulysses Souza <[email protected]>
f8468a8
to
09d931a
Compare
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.
LGTM
Add multitag support for build
Note that with this implementation, building multiple times with different tags and building just one with multiple tags doesn't produce the same output. Check this:
- What I did
Added the option to use multiple tags to an
app image
- How I did it
By turning the
tag
option into a list- How to verify it
- Description for the changelog
Add multitag support for build
- A picture of a cute animal (not mandatory)