Skip to content
This repository was archived by the owner on Jul 18, 2025. It is now read-only.

Conversation

jcsirot
Copy link
Contributor

@jcsirot jcsirot commented Oct 16, 2019

- What I did
This PR add the flag -q to the docker 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:

$ docker app image ls -q
bfd17c107aad
1fa73eccfc5c
bfd17c107aad
bfd17c107aad
916c80e7ab93

A E2E test covering this command has been added

- A picture of a cute animal (not mandatory but encouraged)
image

Signed-off-by: Jean-Christophe Sirot <[email protected]>
@@ -27,6 +28,11 @@ func FromString(s string) (ID, error) {
return ID{s}, nil
}

func FromBundle(bndle *bundle.Bundle) (ID, error) {
Copy link
Contributor

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

Copy link
Contributor

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)

Copy link
Contributor

@eunomie eunomie Oct 17, 2019

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

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor Author

@jcsirot jcsirot Oct 17, 2019

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
Copy link

codecov bot commented Oct 16, 2019

Codecov Report

Merging #695 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f611ee7...fb9defa. Read the comment docs.

@@ -44,6 +63,16 @@ b-simple-app:latest simple
})
}

func TestImageListQuiet(t *testing.T) {
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jcsirot jcsirot force-pushed the add-quiet-flag-image-ls branch from b1ab095 to 06ecfd3 Compare October 17, 2019 15:40
ref2, err := reference.Parse("foo/bar:1.0")
assert.NilError(t, err)
//nolint: errcheck
bundleStore.Store(ref1, &bundle.Bundle{})
Copy link
Contributor

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.

Copy link
Contributor Author

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 <[email protected]>
@jcsirot jcsirot force-pushed the add-quiet-flag-image-ls branch from 06ecfd3 to fb9defa Compare October 18, 2019 11:20
@rumpl rumpl merged commit e4b9653 into docker-archive-public:master Oct 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants