Skip to content
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

Search images in registry in Image Pull form #7928

Closed
wants to merge 18 commits into from

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Jul 3, 2024

Signed-off-by: Philippe Martin [email protected]

What does this PR do?

Search images in registry in Image Pull form

Two fields:

  • first field (required) to enter a search term, the list of image names found will be listed. If the search fails, the text entered is displayed in the list and selectable, so the user can pull an image not discoverable by typing its full name (including its tag)
  • second field (optional) lists the tags found for the image selected in the first field

Screenshot / video of UI

  • search and pull image from docker.io with default tag
  • search and pull image from docker.io with specific tag
  • search and pull image from quay.io
  • pull non discoverable image from ghcr.io discovered tag
  • pull image tag from ghcr.io
search-image.mp4

What issues does this PR fix or reference?

Fixes #971

How to test this PR?

Go to the Images list page, press Pull button, then search images (see video)

  • Tests are covering the bug fix or the new feature

e2e tests have been modified to support the new system. A new parameter has been added to pullImage, two parameters are now necessary:

  • searchTerm: the text entered in the search input
  • imageName: the item selected in the result list

@feloy feloy requested review from benoitf and a team as code owners July 3, 2024 13:06
@feloy feloy requested review from dgolovin and cdrage and removed request for a team July 3, 2024 13:06
@feloy feloy marked this pull request as draft July 3, 2024 13:06
@feloy feloy requested a review from jeffmaury July 3, 2024 13:06
@benoitf
Copy link
Collaborator

benoitf commented Jul 3, 2024

it won't be a required changes but IMHO it will great to have search as long as we type the values and keep the button being 'pull'

so you can just use what you type but also when you're typing something you see the suggestion and you can use it

(same pattern as google search)

@deboer-tim
Copy link
Contributor

Love the capability, but same comments as @benoitf about the UI. Maybe separate PR for renderer so we can merge everything else while we iterate?

Personally I'd like it to automatically search if I pause for a moment, and beneath the input it either confirms you've typed a valid name or shows a list of matches you can click on. This pattern is probably general/reusable enough that we will need an 'AssistedInput' at some point.

I suppose that means I'd prefer keeping it a very simple UI here, maybe somewhere else we add a more explore/browse UI using the same API.

@feloy
Copy link
Contributor Author

feloy commented Jul 3, 2024

I was not sure of the UX for the search, so I have done the simplest (for me) to be able to start the conversation about it (I have added a topic for next UX call).

I have created a PR #7930 for the backend part, so we can merge the backend in the meantime

@feloy feloy force-pushed the feat-971/search-images-native branch from b3d9fa8 to 609062d Compare July 15, 2024 11:41
@feloy feloy marked this pull request as ready for review July 15, 2024 12:29
--input-padding="0"
--item-hover-bg="{colors.get('dropdown-item-hover-bg')?.value}"
--list-background="{colors.get('dropdown-bg')?.value}"
--placeholder-color="{colors.get('input-field-placeholder-text')?.value}"
Copy link
Contributor

Choose a reason for hiding this comment

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

is there any particular reason you are getting the colors from the colors store ?

The following syntax is possible and would makes it easy to migrate to the ui library if we decide to include this components.

Suggested change
--placeholder-color="{colors.get('input-field-placeholder-text')?.value}"
--placeholder-color="var(--pd-input-field-placeholder-text)"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, thanks, this is much better this way; I have made the change

Copy link
Contributor

@deboer-tim deboer-tim left a comment

Choose a reason for hiding this comment

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

I am seeing some styling issues and odd behaviour:

  • No bottom border or rounded outline to start, doesn't match our Input or other controls / not clear to click on it.
  • When I click on the input the popup area appears immediately.
  • Again, styling doesn't match existing popups, looks more like a disabled button.
  • If I start to type and then click outside of the input, it deletes whatever I've typed.
Screen.Recording.2024-07-15.at.2.21.45.PM.mov

@feloy feloy force-pushed the feat-971/search-images-native branch 2 times, most recently from 6e4cdab to 98aa560 Compare July 16, 2024 10:59
@feloy
Copy link
Contributor Author

feloy commented Jul 16, 2024

I am seeing some styling issues and odd behaviour:

  • No bottom border or rounded outline to start, doesn't match our Input or other controls / not clear to click on it.
  • When I click on the input the popup area appears immediately.
  • Again, styling doesn't match existing popups, looks more like a disabled button.
  • If I start to type and then click outside of the input, it deletes whatever I've typed.

Screen.Recording.2024-07-15.at.2.21.45.PM.mov

I have made some changes to the styling.

There is still a problem with the initial focus. The focus is initially set for the select, but is removed after a second or so. I opened an issue rob-balfre/svelte-select#698

@feloy feloy requested a review from deboer-tim July 16, 2024 11:01
Copy link
Contributor

@deboer-tim deboer-tim left a comment

Choose a reason for hiding this comment

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

The styling is getting better. It still doesn't feel like one of our other controls (mostly the padding difference) but is closer. Nit - selection outline on the X is blue.

I'm still finding the behaviour a bit odd, e.g.:

  • if you type docker.io/mongo it still says 'Please enter a value'. You have to pick that option from the list before the warning goes away.
  • Picking an option adds the stars to the entry field like [★10306] ✔.
  • Once you select an option, you can't edit. e.g. if you accidentally select the wrong one you need to start typing or click X and start over, which isn't obvious.

I don't know if I'm setting the bar too high, or this will be a long cycle of trying to force it to have the expected look/behaviour. 🫤

@feloy
Copy link
Contributor Author

feloy commented Jul 16, 2024

I'm still finding the behaviour a bit odd, e.g.:

  • if you type docker.io/mongo it still says 'Please enter a value'. You have to pick that option from the list before the warning goes away.
  • Picking an option adds the stars to the entry field like [★10306] ✔.
  • Once you select an option, you can't edit. e.g. if you accidentally select the wrong one you need to start typing or click X and start over, which isn't obvious.

I don't know if I'm setting the bar too high, or this will be a long cycle of trying to force it to have the expected look/behaviour. 🫤

For most of these points, this is the expected behaviour of the component. If we try to change this behaviour, the result can be very hacky. If we want to have an opinionated behaviour for this component, is seems that it will be easier to rewrite it. WDYT?

Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

LGTM but shouldn't we favor quay.io rather than docker.io 😡

@feloy
Copy link
Contributor Author

feloy commented Jul 17, 2024

The styling is getting better. It still doesn't feel like one of our other controls (mostly the padding difference) but is closer. Nit - selection outline on the X is blue.

I changed the styling, to adjust the height of the input, and the selection outline.

@feloy feloy force-pushed the feat-971/search-images-native branch from 2c82ff1 to 71f76e2 Compare July 17, 2024 14:25
@feloy feloy requested a review from deboer-tim July 17, 2024 14:38
@feloy feloy force-pushed the feat-971/search-images-native branch 2 times, most recently from bfe2a41 to b3bc8fa Compare July 18, 2024 08:12
@feloy
Copy link
Contributor Author

feloy commented Jul 18, 2024

When trying to fix e2e tests, I realize that the user cannot select a specific tag of an image...

@feloy
Copy link
Contributor Author

feloy commented Jul 18, 2024

The search endpoint of some registries do not work as expected, for example quay.io which is not able to find an image by giving its org repo (here org=podman-desktop-demo and repo=podify-demo-backend):

$ podman search quay.io/podify-demo-backend
NAME                                             DESCRIPTION
quay.io/podman-desktop-demo/podify-demo-backend  
$

but:

$ podman search quay.io/podman-desktop-demo/podify-demo-backend
$

The same search works on docker.io:

$ podman search docker.io/feloy/hello-app 
NAME                       DESCRIPTION
docker.io/feloy/hello-app  
$

@feloy
Copy link
Contributor Author

feloy commented Jul 30, 2024

Did you try the fifth example in the demo video? (by typing the complete image tag in the first field). I hesitate to add an Info icon with a tooltip to explain the different possibilities

The pull button does not seems to unlock?
image

Ah, this case is not supported, when the search returns something
It should work when you type: axel7083/remote-dev:frontend
I'll probably have to change, to always display in the list of results what the user has typed, so it can be selected. For now, it is displayed only if the search fails for some reason, but this does not cover your case

Would it be possible to uses the token of the registries to list the private repositories as well?

Yes I think so. I'll work on this

@feloy
Copy link
Contributor Author

feloy commented Jul 31, 2024

Did you try the fifth example in the demo video? (by typing the complete image tag in the first field). I hesitate to add an Info icon with a tooltip to explain the different possibilities

The pull button does not seems to unlock?
image

Ah, this case is not supported, when the search returns something
It should work when you type: axel7083/remote-dev:frontend
I'll probably have to change, to always display in the list of results what the user has typed, so it can be selected. For now, it is displayed only if the search fails for some reason, but this does not cover your case

Would it be possible to uses the token of the registries to list the private repositories as well?

Yes I think so. I'll work on this

@axel7083
After a few researches, I cannot find a way to make a search request including private repos.

For the moment, to be able to list tags for private repos, you will need to add a registry index.docker.io in your Podman Desktop registries (as a workaround for #8293).

I have added a commit to add to the Select items the image name entered by the user, when it is not returned by the search, so the user can pull any image

@feloy feloy force-pushed the feat-971/search-images-native branch from da11f42 to 069e59c Compare July 31, 2024 09:49
@deboer-tim
Copy link
Contributor

The ability to search images and see all the tags is awesome, but while testing I'm still noticing minor issues with the UI:

  • When entering the form, image name input flashes selected -> unselected.
  • Nice popup description when you click the form, but doesn't quite match what we do anywhere else, and appears even when you have entered something valid.
  • If I select an image name from the list I can't edit it, only clear and start over.
  • If I type in an image name or copy/paste the tags are never filled and Pull isn't enabled, I have to select the same name from the dropdown list. (extra step)
  • If I clear the image name and then type or copy/paste a valid name, the validation still says 'Please enter a value', because you have to select the identical text from the list first.
  • If I select an image tag and then clear the image name, the tag isn't cleared. So it will say it's going to pull image X tag Y, but it actually pulls latest.

I'm being very picky, but despite the excellent work @feloy has done to make it fit in the component just isn't feeling super usable or natural to me. I'm happy to keep reviewing and if others feel it's ok I'm not going to block, but also wondering if we should discuss other options.

Signed-off-by: Philippe Martin <[email protected]>
Signed-off-by: Philippe Martin <[email protected]>
Signed-off-by: Philippe Martin <[email protected]>
Signed-off-by: Philippe Martin <[email protected]>
Signed-off-by: Philippe Martin <[email protected]>
Signed-off-by: Philippe Martin <[email protected]>
Signed-off-by: Philippe Martin <[email protected]>
Signed-off-by: Philippe Martin <[email protected]>
use the same color as non disabled, to stick with what the Input component is doing
Signed-off-by: Philippe Martin <[email protected]>
Signed-off-by: Philippe Martin <[email protected]>
@feloy feloy force-pushed the feat-971/search-images-native branch from 069e59c to 190a713 Compare September 4, 2024 09:45
Signed-off-by: Philippe Martin <[email protected]>
@feloy
Copy link
Contributor Author

feloy commented Sep 4, 2024

One problem highlighted by e2e tests:

If an image has more than 100 tags, only 100 tags will be returned, and the one wanted by the user may not be in the list. In this case, there is no way for the user to specify a non-listed tag

@feloy
Copy link
Contributor Author

feloy commented Sep 5, 2024

Following today's discussion, I have found this component https://github.com/metonym/svelte-typeahead which behaves more like we would like (just copy the content of the selected entry in the input, without having to clear the entry with the X to continue to edit it). The problem is that is depends on the svelte-search component, when we are using our own input component anywhere else. The sources seem simple enough to get inspiration from it and/or remove the dependency from the svelte-search to replace it with our own input.
cc @deboer-tim

(edited to copy-paste the correct components)

@cdrage
Copy link
Contributor

cdrage commented Sep 5, 2024

I'm trying this with a private registry I use (hosted on forgejo locally), and I'm getting some odd issues.

The first is a 401 error in the console which is expected, but there was no warning in the UI:

VM5:62 main ↪️ error getting tags of image git.k8s.land/foobar Error: Response code 401 (Unauthorized)
    at Request.<anonymous> (/Users/cdrage/syncthing/dev/electron/podman-desktop/packages/main/dist/index.cjs:75229:33)
    at Object.onceWrapper (node:events:634:26)
    at Request.emit (node:events:531:35)
    at Request._onResponseBase (/Users/cdrage/syncthing/dev/electron/podman-desktop/packages/main/dist/index.cjs:74792:14)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async Request._onResponse (/Users/cdrage/syncthing/dev/electron/podman-desktop/packages/main/dist/index.cjs:74828:7)
    ```
    
Another is that when I enter the image, and then I "unclick" to another section. When I go back to the image name, the cursor goes to the beginning, and I cannot append the image name to pull.

If you can see, I do "git.k8s.land/foobar" and then I want to go back to the input to append something like /foobar/test to the URL, but it forces me to retype the entire thing:


https://github.com/user-attachments/assets/7ecf9a18-89b7-47a4-9bf1-cd492ee6b710


@cdrage
Copy link
Contributor

cdrage commented Sep 5, 2024

I am also getting multiple "An object could not be cloned" errors in the console:

VM5:62 main ↪️ Error occurred in handler for 'image-registry:searchImages': Error: An object could not be cloned.
    at WebContents.<anonymous> (node:electron/js2c/browser_init:2:85456)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    ```

@feloy feloy marked this pull request as draft September 6, 2024 11:32
@feloy
Copy link
Contributor Author

feloy commented Sep 9, 2024

Closing in favor of #8786

@feloy feloy closed this Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to browse images on remote repositories
6 participants