Fix incomplete Mounts attributes #25697
Conversation
openshift-ci
bot
added
release-note
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: D3vil0p3r The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Antonio <[email protected]>
D3vil0p3r
changed the title
| @@ -147,7 +147,7 @@ type ContainerRootFSConfig struct { | |||
| // Mounts contains all additional mounts into the container rootfs. | |||
| // It is presently only used for the container's SHM directory. | |||
| // These must be unmounted before the container's rootfs is unmounted. | |||
| Mounts []string `json:"mounts,omitempty"` | |||
There was a problem hiding this comment.
This is going to break upgrades from previous versions - you can't deserialize a JSON string into a define.InspectMount. It should be possible to, instead of this, make a func (c *Container) InspectMounts() ([]define.InspectMount, error) that takes the libpod ContainerConfig and converts it into InspectMount in the same way Inspect does (with a minor refactor, you could use the same code for both). That would probably be sufficient for your needs here, no?
There was a problem hiding this comment.
Yes I can try. I understand it would be a breaking change. As I reported in my comment below, it would be great if, at least, it could be applied on the next major release.
There was a problem hiding this comment.
in the same way Inspect does (with a minor refactor, you could use the same code for both)
to which specific Inspect function you refer? This one?
podman/libpod/container_inspect.go
Line 39 in 490eb47
There was a problem hiding this comment.
Overall as I said on the issue we simply cannot do this, it will break everyone.
And then you may only care about mounts right now but the next person wants something else, to me this is simply not a sustainable fix in any way or form.
This needs a larger decision on what info the list API should carry because right now it is only aimed at the podman ps output.
| @@ -41,7 +42,7 @@ type ListContainer struct { | |||
| // Labels for container | |||
| Labels map[string]string | |||
| // User volume mounts | |||
| Mounts []string | |||
| Mounts []pod_define.InspectMount | |||
There was a problem hiding this comment.
As I mentioned on the issue these are breaking changes to a user facing API and cannot be done. We canont just break things.
There was a problem hiding this comment.
My 2cent: I understand, but at least for a major release. Each major release of all software could introduce breaking changes, but the fear to keep everything "unbreakable" just stops any improvement on the software itself (and on podman). But still, those "Mounts" attributes as-is, that are just destination directories, are unusable, simply because the same call does not return the related "Source", it is just an example.
For this particular case, I'm happily spending my small free time to enrich those data. I don't do this only because I need, I do because I "believe" on podman project and I use it as a standard over docker
There was a problem hiding this comment.
but the fear to keep everything "unbreakable" just stops any improvement on the software itself (and on podman)
This isn't a fear, this is plain and simple a breaking API change. An old remote client would be unable to parse this json and thus break podman ps and the other way around the same way.
We cannot just break API users just because you happen to need different data in the field. There are ways to introduce new features without breaking existing ones, we do that all the time. New options and new fields would be fine (still assumes they make sense overall and are maintainable) but you cannot break the format of an existing one.
Yes we can break things in a major release but a new major release is still far away, not even planned yet. And if we like to break things that there must be a strong reason and we should do a careful audit of similar problems in the list API. Otherwise we have the same issue on a different field again.
For this particular case, I'm happily spending my small free time to enrich those data. I don't do this only because I need, I do because I "believe" on podman project and I use it as a standard over docker
I am thankful that you take the time to contribute to the project, but compatibility is an important topic for us, so you would need to find another way to move this forward. Adding a new API field that has the data you need could be an option.
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Does this PR introduce a user-facing change?
Fix #24878