-
Notifications
You must be signed in to change notification settings - Fork 177
Conversation
Signed-off-by: Jean-Christophe Sirot <jean-christophe.sirot@docker.com>
@@ -27,6 +28,11 @@ func FromString(s string) (ID, error) { | |||
return ID{s}, nil | |||
} | |||
|
|||
func FromBundle(bndle *bundle.Bundle) (ID, error) { |
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.
It’s an odd name
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.
Indeed. What about attaching a getID() function to Bundle ?
(sorry, my brain is still object-oriented)
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.
wasn't the remark about bndle
?
Everywhere in the code it's bndl
, bndle
is used only inside internal/store/bundle.go
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.
Nah, FromBundle
is an odd name. When called it's: store.FromBundle(..)
and you get ... an ID. Confusing to say the least
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 the package is not the right one.
Something like id.FromString
and id.FromBundle
(or digest)
Not store.FromBundle
as we don't return a store.
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.
store.FromString
returns an ID as well... Maybe we should rename them into IDFromString
and IDFromBundle
?
Codecov Report
@@ Coverage Diff @@
## master #695 +/- ##
=======================================
Coverage 71.77% 71.77%
=======================================
Files 56 56
Lines 2983 2983
=======================================
Hits 2141 2141
Misses 567 567
Partials 275 275 Continue to review full report at Codecov.
|
@@ -44,6 +63,16 @@ b-simple-app:latest simple | |||
}) | |||
} | |||
|
|||
func TestImageListQuiet(t *testing.T) { |
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 a unit test would be easier for this feature, mocking the bundle store to configure it the way you want.
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.
A unit test has been added
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
b1ab095
to
06ecfd3
Compare
internal/commands/image/list_test.go
Outdated
ref2, err := reference.Parse("foo/bar:1.0") | ||
assert.NilError(t, err) | ||
//nolint: errcheck | ||
bundleStore.Store(ref1, &bundle.Bundle{}) |
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.
You should assert on the error here.
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.
Fixed
Signed-off-by: Jean-Christophe Sirot <jean-christophe.sirot@docker.com>
06ecfd3
to
fb9defa
Compare
- What I did
This PR add the flag
-q
to thedocker app image ls
command. When this flag is set to true only the app image id are printed.- How to verify it
Run
docker app image ls -q
. The output must look like:A E2E test covering this command has been added
- A picture of a cute animal (not mandatory but encouraged)