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

feat: show manifests in image list #7227

Merged
merged 2 commits into from
May 30, 2024
Merged

Conversation

cdrage
Copy link
Contributor

@cdrage cdrage commented May 15, 2024

feat: show manifests in image list

What does this PR do?

  • Shows manifest in the image list
  • Does NOT show any actions / be able to click (yet)
  • Allows you to click and view details.

Screenshot / video of UI

Screen.Recording.2024-05-21.at.11.06.01.AM.mov

What issues does this PR fix or reference?

Closes #6529
Closes #7255

How to test this PR?

  • Tests are covering the bug fix or the new feature
  1. Create a manifest through the GUI or podman build --platform linux/arm64,linux/amd64 --manifest testmanifest123
  2. View the manifest in image list.

Signed-off-by: Charlie Drage charlie@charliedrage.com

@cdrage cdrage requested review from benoitf and a team as code owners May 15, 2024 19:58
@cdrage cdrage requested review from axel7083 and lstocchi and removed request for a team May 15, 2024 19:58
@cdrage
Copy link
Contributor Author

cdrage commented May 15, 2024

FYI it is intentional that there are no actions shown / unable to click for more details / no delete button. As those are in separate PR's in the epic: #6526 specifically issues: #7041 and #7181

@cdrage cdrage force-pushed the add-manifests branch 3 times, most recently from 9c9e970 to 9140cd6 Compare May 16, 2024 13:33
Copy link
Collaborator

@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 think it's curious/annoying that manifests are images in the API, and makes it so that something like ImageColumnActions needs to be shared. :-/

I think this is a good place to go with the first PR, but:

  • In the containers view we have "my-pod (pod)", but here you used a badge. We obviously didn't have badges when we did the containers view, but I think we need to decide with design the right way to identify 'different things in a table' is and make them consistent. For simplicity/consistency maybe I'd rather use '(manifest)' until we know.
  • The icon should be different from a regular image.
  • Manifests should have details pages (but can be a separate issue).
  • We should allow them to expand/collapse with the images that are in them, but that is post Table component support for parent/children #7209.

I don't want to hold this up too much, but I think we should at least have a direction on 1/2 and issue for 3 before we merge.

@cdrage
Copy link
Contributor Author

cdrage commented May 17, 2024

I think it's curious/annoying that manifests are images in the API, and makes it so that something like ImageColumnActions needs to be shared. :-/

I think this is a good place to go with the first PR, but:

  • In the containers view we have "my-pod (pod)", but here you used a badge. We obviously didn't have badges when we did the containers view, but I think we need to decide with design the right way to identify 'different things in a table' is and make them consistent. For simplicity/consistency maybe I'd rather use '(manifest)' until we know.
  • The icon should be different from a regular image.
  • Manifests should have details pages (but can be a separate issue).
  • We should allow them to expand/collapse with the images that are in them, but that is post Table component support for parent/children #7209.

I don't want to hold this up too much, but I think we should at least have a direction on 1/2 and issue for 3 before we merge.

I totally agree, let's talk about it in the UX call. There are a lot of other missing pieces that should be worked on before we merge the "showing in list" support.

For this PR I will:

  • Add (manifest) similar to (my-pod) instead of a badge
  • Different icon (will ask you or @ekidneyrh on advice what we should do)

I thought I had opened a "details" issue for manifest, but I had not. I've opened up issue #7255 and added it to the epic #6526

Maybe we should keep this as draft / WIP until we get the details page in too so we at least have the basic functionality of manifests.

@cdrage cdrage marked this pull request as draft May 17, 2024 20:52
@cdrage cdrage force-pushed the add-manifests branch 3 times, most recently from 7457715 to e2aaadb Compare May 21, 2024 15:04
@cdrage cdrage marked this pull request as ready for review May 21, 2024 15:06
@cdrage
Copy link
Contributor Author

cdrage commented May 21, 2024

This PR is ready for review with the exception of the new icon being added, which can be in a follow up PR.

I've made the following changes:

  • New folder / files, based on how we do ImageDetails.svelte
  • Added a BASIC manifest summary page, a more advanced summary page based on using inspectManifest will be added via issue: Manifests "more in depth" details page. #7270
  • Added tests

Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

Tested and works as described. LGTM

packages/renderer/src/lib/image/ImageDetailsHistory.svelte Outdated Show resolved Hide resolved
@ekidneyrh ekidneyrh mentioned this pull request May 23, 2024
@cdrage cdrage requested a review from deboer-tim May 27, 2024 12:35
@cdrage cdrage force-pushed the add-manifests branch 3 times, most recently from 849c23d to 3e1dd3d Compare May 28, 2024 20:38
### What does this PR do?

* Shows manifest in the image list
* Does NOT show any actions / be able to click (yet)
* SHows "manifest" as a badge.

### Screenshot / video of UI

<!-- If this PR is changing UI, please include
screenshots or screencasts showing the difference -->

### What issues does this PR fix or reference?

<!-- Include any related issues from Podman Desktop
repository (or from another issue tracker). -->

Closes containers#6529

### How to test this PR?

<!-- Please explain steps to verify the functionality,
do not forget to provide unit/component tests -->

- [X] Tests are covering the bug fix or the new feature

1. Create a manifest through the GUI or `podman build --platform
   linux/arm64,linux/amd64 --manifest testmanifest123`
2. View the manifest in image list.

Signed-off-by: Charlie Drage <charlie@charliedrage.com>
@cdrage
Copy link
Contributor Author

cdrage commented May 30, 2024

Manifest icon has now been added and this PR is ready for another review @lstocchi @deboer-tim

Signed-off-by: Charlie Drage <charlie@charliedrage.com>
Copy link
Collaborator

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

LGTM, worked well in testing.

@cdrage cdrage merged commit 6acaed2 into containers:main May 30, 2024
8 checks passed
@podman-desktop-bot podman-desktop-bot added this to the 1.11.0 milestone May 30, 2024
cdrage added a commit to cdrage/podman-desktop that referenced this pull request Jun 19, 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.

Manifest basic "details" page Manifest feature: List manifests on image list screen
4 participants