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

Volumes support #957

Draft
wants to merge 20 commits into
base: main
Choose a base branch
from
Draft

Volumes support #957

wants to merge 20 commits into from

Conversation

strzinek
Copy link

Basic support for Podman volumes management:

  • Container details - Mounts
  • Container details - ENV
  • Volumes list
  • Volume details
  • Create and remove volume

* First attempt to support podman volumes
* Container details - Mounts
* Container details - ENV
* Volumes list 
* Create and remove volume
* Volume details
Copy link
Member

@garrett garrett left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for the PR.

It's a great start and seems mostly implemented feature-wise, but it's currently incomplete and still needs a bit of work.


As-is, this doesn't fit the design of Cockpit Podman, as you flipped the order of images and containers. As containers are derived from images, the images need to be on top. Additionally, the containers list can get very long, so everything below it might not be seen. This is why the design has images at the top and is expandable (and collapsed by default).

(Design doc @ cockpit-project/cockpit#16059)

Here's what it looks like in the PR right now:

Screenshot 2022-04-12 at 10-36-32 Podman containers - garrett@Bolt

We might want to use side-by-side cards at the top, similar to Cockpit-Machines. In widescreen, they're side-by-side. When space is constrained, they wrap and stack. (This can be done with PatternFly Flex with breakpoints.):

Screenshot 2022-04-12 at 10-51-00 Virtual Machines - garrett@Bolt

Screenshot 2022-04-12 at 10-51-16 Virtual Machines - garrett@Bolt


While it works when maximized on a high-res screen and also collapses down on mobile, the list doesn't work in standard desktop sizes:

Screenshot 2022-04-12 at 10-36-46 Podman containers - garrett@Bolt

Here it is in huge, just for reference of others viewing the PR:

Screenshot 2022-04-12 at 10-37-36 Podman containers - garrett@Bolt


Create volume says "Create container" and has tabs. (It's not obvious what this does. I guess it creates volumes in a standard location, mainly for use in containers later?)

It shouldn't have tabs.

Screenshot 2022-04-12 at 10-37-05 Podman containers - garrett@Bolt

I'm assuming you'll add various options as listed @ https://docs.podman.io/en/latest/markdown/podman-volume-create.1.html too? (This could be handled in follow-up PRs.)


Just as a point of reference, prune and delete are the only options at the moment:

Screenshot 2022-04-12 at 10-38-37 Podman containers - garrett@Bolt

Screenshot 2022-04-12 at 10-38-45 Podman containers - garrett@Bolt

@strzinek
Copy link
Author

Thanks @garrett for your review, I'll incorporate your valid comments.

@strzinek strzinek requested a review from garrett April 12, 2022 17:54
@jelly
Copy link
Member

jelly commented Apr 20, 2022

As volumes allow for purning unused volumes, shouldn't we also show which volume is unused and used by a container similar to images?

@strzinek
Copy link
Author

I agree, @jelly, but there is no volume usage info present in API, unlike for images. API does not even return volume ID's, only names. List of unused volumes could be reconstructed by going through all the containers on client side, I think. Or is there any easier way?

@garrett
Copy link
Member

garrett commented May 4, 2022

Volumes details are empty. Should there be anything there? If not, they shouldn't be expandable. And if there's only one tab of information, there shouldn't be a tab.

Screenshot 2022-05-04 at 11-53-16 Podman containers - garrett@Bolt


Also, PF Cards now have the ability to be expandable. Perhaps we should use this for both Images and Volumes. But we can address that in a follow-up. It's just something to keep in mind as it would impact the way volumes look on the page.

https://www.patternfly.org/v4/components/card#expandable
https://www.patternfly.org/v4/components/card/react-demos/#horizontal-card-grid

@marusak
Copy link
Member

marusak commented Jan 25, 2024

@strzinek Hey! Seems this fall of our radar and now has conflicts. Is this still something you are interested in and able to adjust to currect podman?

@marusak marusak marked this pull request as draft January 25, 2024 09:53
@marusak marusak removed the request for review from garrett January 25, 2024 09:53
src/VolumeCreateModal.jsx Fixed Show fixed Hide fixed
@strzinek
Copy link
Author

strzinek commented Feb 17, 2024

@strzinek Hey! Seems this fall of our radar and now has conflicts. Is this still something you are interested in and able to adjust to currect podman?

I resolved merge conflicts and removed changes to container details, as these have since been implemented by someone else.

TODO:

  • adapt to changes in the project - it may not work correctly, I have not tested it yet
  • write basic tests for volumes

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.

4 participants