Add podman search command #241

umohnani8 · 2018-01-18T17:49:13Z

podman search queries a registry for a matching image and prints the output.
I added a new flag called "registry" giving the user the option
to search a specific registry if they don't want to search all
their default registries.

Signed-off-by: umohnani8 [email protected]

umohnani8 · 2018-01-18T17:49:47Z

Need containers/image#406 to be merged in first.
Adding tests.
@rhatdan @mheon PTAL

umohnani8 · 2018-01-18T18:57:18Z

Added tests

rhatdan · 2018-01-18T18:54:12Z

cmd/podman/search.go

Your not returning anything?

It is returning genericParams, which has already been defined with the return type in the function definition.

func searchToGeneric(params []searchParams) (genericParams []interface{}) {

Don't you need to say return genericParams? If not you should because it makes it easier to understand for dummies like me.

no you don't, and fixed.

rhatdan · 2018-01-18T18:54:54Z

cmd/podman/search.go

I think you can use v.NumField() to prebuild the values struct, and things work a little faster.

rhatdan · 2018-01-18T18:55:41Z

cmd/podman/search.go

context.TODO?

context.TODO() gives you an empty context when not sure which context to use. It is needed by the functions in containers/image/docker/docker_client.go.

Ok, never seen that interface before,

rhatdan · 2018-01-18T18:59:52Z

cmd/podman/search.go

You could shrink this by saying

if len(arr) == 2 && arr[1] != "true" { filterParams.isAutomated = false } else { filterParams.isAutomated = true }

rhatdan · 2018-01-18T19:00:10Z

cmd/podman/search.go

Same change as above.

rhatdan · 2018-01-18T19:06:29Z

cmd/podman/search.go

Don't need, default booleans to true.

rhatdan · 2018-01-18T19:06:45Z

cmd/podman/search.go

Default to True and eliminate the set flag.

rhatdan · 2018-01-18T19:07:39Z

cmd/podman/search.go

Should not need isAutomatedSet and return result.IsAutomated

I don't think I am getting what you mean. If I don't have the isAutomatedSet field, and just compare the isAutomated field of the result to the filter, I will get only the results that have this set to false or true based on the value of filter.isAutomated. But I want all the results regardless of the isAutomated value when the is-automated filter is not set.

if isAutomated is defaulted to true. Then only changed if the user specifies to remove it with automated == false.

You are always printing it unless the user tells you not to, so default to true and set it to false when the user tells, then you don't need the rest of the code.

If that is the case, then when the user only wants images with isAutomated true, they will get all the images regardless of the images isAutomated value. It will work for the false case, but not the true case.
These are the cases:

--filter=isAutomated=true -> Want all images with isAutomated set to true

--filter =isAutomated=false -> Want all images with isAutomated set to false

no filter set -> Want all images regardless of the image's isAutomated value

Ok, I still think we could get by with one variable.
Automated String
Set it to "true", "false" or "" and that gets your three states.

I would go with *bool - that gives us true, false, and nil

Right, forgot we could use pointers. Fixed

rhatdan · 2018-01-18T19:07:55Z

cmd/podman/search.go

Same reasoning as above.

rhatdan · 2018-01-18T19:13:14Z

Update readme.md and transfer.md

TomSweeneyRedHat · 2018-01-18T19:43:34Z

#230 really changed up the README.md, could changes here be held until that's merged?

TomSweeneyRedHat · 2018-01-18T19:47:30Z

docs/podman-search.1.md

which config file?

Yes specify the file name.

TomSweeneyRedHat · 2018-01-18T19:48:15Z

docs/podman-search.1.md

is-automated?

TomSweeneyRedHat · 2018-01-18T19:48:57Z

docs/podman-search.1.md

putput to output

TomSweeneyRedHat · 2018-01-18T19:50:16Z

docs/podman-search.1.md

what makes an image "official". A bit more verbiage here or the opening description would be nice.

Of course this only means that someone paid Docker to make it official...

Should I add a description or let it be?

Docker does not describe it and it is for the benefit of Docker to call things official. So I would not describe it. I am not sure if we declare Red Hat images as Official. So lets leave this alone.

TomSweeneyRedHat · 2018-01-18T19:51:57Z

cmd/podman/search.go

Can/should we allow them to specify more than one registry?

I don't see why not. This is a feature I added as podman search searches multiple registries based on how many are set as default registries in the config file. This was just a way to give the user to specify which registry he/she wants to search without dealing with the lag of searching all default registries.
If we want to allow specifying more than one, it can be done. @rhatdan what do you think?

The config file being /etc/containers/registries.conf

Yes allow me to specify multiple registries, I can even specify ones that are not in registries.conf

TomSweeneyRedHat · 2018-01-18T19:56:05Z

docs/podman-search.1.md

suggests "results of" to "results from". If you have 10 results in a registry and limit is set to 3, which 3 do you get? Oldest, newest, alphabetical, other? It would probably be nice to talk about ordering here. Might also be nice to have a way to sort the output.

There is no particular order, just the order in which the api returns the results. docker search sorts it based on the star count, highest to lowest. I didn't bother to do that as that would make search slower, and it is already a bit slow. I don't think podman should care that much about the star count, looks to be mainly a docker hub thing. @rhatdan what are your thoughts? I can definitely clarify that the output is ordered based on the order of the results from the api.

Lets not sort for now, until someone complains.

rhatdan · 2018-01-18T19:12:49Z

completions/bash/podman

Need search below.

umohnani8 · 2018-02-05T17:20:37Z

vendoring in latest containers/image with support for searching registries.
This can get in once tests pass.
@rhatdan @mheon PTAL

rhatdan · 2018-02-05T17:56:51Z

LGTM, but we are stuck with the gomd2man issues.

TomSweeneyRedHat · 2018-02-06T17:52:12Z

docs/podman-search.1.md

A format example would be a nice to have here.

TomSweeneyRedHat · 2018-02-06T17:54:23Z

One suggestion, otherwise LGTM. Test failures are unrelated, should we retest?

Latest containers/image has support for searching registries. Signed-off-by: umohnani8 <[email protected]>

podman search queries a registry for a matching image and prints the output. I added a new flag called "registry" giving the user the option to search a specific registry if they don't want to search all their default registries. Signed-off-by: umohnani8 <[email protected]>

TomSweeneyRedHat

LGTM, assuming the network testing gremlins stay far away.

rhatdan · 2018-02-06T18:09:29Z

@rh-atomic-bot r+

rh-atomic-bot · 2018-02-06T18:09:30Z

📌 Commit 9e44195 has been approved by rhatdan

rh-atomic-bot · 2018-02-06T18:09:34Z

⌛ Testing commit 9e44195 with merge 0d7e6fa...

podman search queries a registry for a matching image and prints the output. I added a new flag called "registry" giving the user the option to search a specific registry if they don't want to search all their default registries. Signed-off-by: umohnani8 <[email protected]> Closes: #241 Approved by: rhatdan

rh-atomic-bot · 2018-02-06T18:36:24Z

☀️ Test successful - status-papr
Approved by: rhatdan
Pushing 0d7e6fa to master...

umohnani8 force-pushed the podman_search branch 3 times, most recently from 2c626e1 to 5aa5dd4 Compare January 18, 2018 18:56

rhatdan reviewed Jan 18, 2018

View reviewed changes

TomSweeneyRedHat reviewed Jan 18, 2018

View reviewed changes

rhatdan reviewed Jan 18, 2018

View reviewed changes

umohnani8 force-pushed the podman_search branch 8 times, most recently from 1358783 to 1ad156e Compare January 24, 2018 17:36

umohnani8 force-pushed the podman_search branch from 1ad156e to 4257224 Compare February 5, 2018 17:18

umohnani8 changed the title ~~[WIP] Add podman search command~~ Add podman search command Feb 5, 2018

umohnani8 force-pushed the podman_search branch 3 times, most recently from f2f44ce to 3fc58b7 Compare February 6, 2018 15:23

umohnani8 force-pushed the podman_search branch from 3fc58b7 to 056264b Compare February 6, 2018 17:04

TomSweeneyRedHat reviewed Feb 6, 2018

View reviewed changes

umohnani8 added 2 commits February 6, 2018 12:59

Vendor in latest containers/image

5a88e8f

Latest containers/image has support for searching registries. Signed-off-by: umohnani8 <[email protected]>

umohnani8 force-pushed the podman_search branch from 056264b to 9e44195 Compare February 6, 2018 18:01

TomSweeneyRedHat reviewed Feb 6, 2018

View reviewed changes

rh-atomic-bot closed this in 1a48a7a Feb 6, 2018

umohnani8 deleted the podman_search branch May 16, 2018 15:43

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 27, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2023

Add podman search command #241

Add podman search command #241

Uh oh!

Conversation

umohnani8 commented Jan 18, 2018

Uh oh!

umohnani8 commented Jan 18, 2018

Uh oh!

umohnani8 commented Jan 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Jan 18, 2018

Uh oh!

TomSweeneyRedHat commented Jan 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!