Support listing volumes#1959
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
I like this, thanks! Another idea I have is to show which containers use the volume. Maybe a popover for the text in the "Used by" column? |
Right we have this for images!
So maybe an expandable makes sense here. |
|
@jelly this would be a great addition to cockpit-podman, is there anything left to do before merging this PR? I'd be willing to help with implementing the display of the size of the volumes later on. |
|
Any update on this? |
|
@jelly wanna rebase?
I think we should support these in the same PR. I think the initial work looks good, I'm wondering if we should do anything with volumes without a name, e.g. those created with just 
|
jelly
force-pushed
the
list-volumes
branch
2 times, most recently
from
bee2e7e to
8b8726c
Compare
Venefilyn
left a comment
Venefilyn
left a comment
There was a problem hiding this comment.
Overall looks good! Couple of comments going through the commits
| initVolumes(con) { | ||
| return client.getVolumes(con) | ||
| .then(volumesList => { | ||
| console.log(volumesList); |
There was a problem hiding this comment.
Should be removed from the commit
| import ContainerHeader from './ContainerHeader.jsx'; | ||
| import Containers from './Containers.jsx'; | ||
| import Images from './Images.jsx'; | ||
| import Volumes from './Volumes.jsx'; |
There was a problem hiding this comment.
Hasn't been added on or before this commit
| <ModalFooter> | ||
| <Button id="btn-volme-delete" variant="danger" | ||
| onClick={() => handleRemoveVolume()}> | ||
| {force ? _("Force delete volume") : _("Delete volume")} |
There was a problem hiding this comment.
Force is never used atm, state is never modified
| <FlexItem grow={{ default: 'grow' }}> | ||
| <Flex> | ||
| <CardTitle> | ||
| <h2 className="containers-images-title">{_("Volumes")}</h2> |
There was a problem hiding this comment.
Should be
| <h2 className="containers-images-title">{_("Volumes")}</h2> | |
| <Content component={ContentVariants.h1} className="containers-volumes-title">{_("Volumes")}</Content> |
Currently the heading differs from images and containers. The class name also needs a change
| Object.entries(prevState.volumes || {}).forEach(([id, volume]) => { | ||
| if (volume.uid !== con.uid) | ||
| copyVolumes[id] = volume; | ||
| }); |
There was a problem hiding this comment.
This seems expensive. Can we keep an index of everything we found so far or otherwise not do a heavy Object.entries on existing state?
There was a problem hiding this comment.
This is how our state works for containers, images and now also volumes. However I don't understand your concern here iterating over an array is not that slow in JavaScript and we do need new state (ie an object).
If we want to improve this I am not sure how as I don't understand your proposal, getting the volumes basically resets the state for the current logged in user (uid).
The variable naming is a bit confusing as it is in fact an object.
6 participants
Rought mockup with basic volume listing
Let's add a basic listing and then delete support.
Tests needed
As follow up:
Related to: #2342