cmd/list: Make listing of containers/images more robust #503
Conversation
|
Build failed.
|
HarryMichal
force-pushed
the
get-listing-strong
branch
from
f270c7f to
afd6286
Compare
|
Build failed.
|
|
I think that the approach that toolbox is taking to parsing json output is generally not optimal - and not the way that pretty much every golang project I've looked at does it. Unmarshaling to a map of interfaces puts the checking (and error handling as is added here) at the point of use, which is both error-prone and hard to maintain. We talked to the libpod maintainers some, and their recommendation was to use the libpod bindings that talk over a local http socket. Using this requires the fedora packaging to be updated to automatically create the socket for all users using a systemd user preset, and also would be a pretty major refactoring. I think a short term fix would be, to, e.g. change GetContainers to return []Container where the Container type has just the fields that you actually care about, and unmarshal directly to that list. Then the json package will do the type checking upfront. It even has some ability to skip fields with a type mismatch:
|
HarryMichal
force-pushed
the
get-listing-strong
branch
from
afd6286 to
91a3cb0
Compare
|
Build failed.
|
HarryMichal
force-pushed
the
get-listing-strong
branch
from
91a3cb0 to
c2c3b2c
Compare
|
Build failed.
|
HarryMichal
added
3. Enhancement
HarryMichal
force-pushed
the
get-listing-strong
branch
from
c2c3b2c to
94daaf5
Compare
|
Build failed.
|
There was a problem hiding this comment.
It would be slightly better to structure this as a commit that just switches around the structure to use JSON unmarshaling and tries to keep the logic the same as before, followed by another commit that adds the additional compat handling for the new 'podman images' breakage. But that's entirely optional - up to how comfortable you are restructuring git commits.
src/cmd/list.go
Outdated
| c.Name = raw.Names[0] | ||
| } | ||
|
|
||
| if raw.Status != "" { |
There was a problem hiding this comment.
From the discussion in containers/podman#6980 I think this is an exception to old => new - a human Status is what we want and it sounds like it will be added back.
HarryMichal
force-pushed
the
get-listing-strong
branch
from
94daaf5 to
c79c9d1
Compare
HarryMichal
changed the title
|
Build failed.
|
|
System tests are still failing on Fedora Rawhide but that is due to a different issue, not #502 that is fixed by this PR. |
|
I just tried this with Podman v1.9.3 and there's a problem with listing containers due to: I'll adjust this accordingly. |
HarryMichal
force-pushed
the
get-listing-strong
branch
2 times, most recently
from
9970c70 to
2fcb635
Compare
|
Build failed.
|
|
Latest version looks good to me! |
This package will be used for generating a human-readable representation of elapsed time (e.g., "5 minutes"). containers#503
Every time Podman changes their JSON API Toolbox breaks horribly. That
is caused by the combination of decoding the JSON purely by hand and by
the complete lack of type assertions to make the process stable.
I previously didn't know that unmarshalling with Go works on the 'do the
best job it can' logic (thank you Owen!). This makes the need to check
for subtle changes in the names of fields go away and type checking a
bit more bareable (marking the questioned field as interface{} and then
immediatelly type switch).
If even now an existing field does not hold an expected value the field
will remain blank.
To take this a bit further, I created two types (toolboxImage and
toolboxContainer) that are used by both functions for returning the list
of images and containers. Instead of using getters for properties that
need some checks or adjustments this uses a custom unmarshaling function
(Owen's idea; thanks!).
The work around populating the two new types is a bit hacky (marshalling
and already unmarshalled JSON - that was unmarshalled to
[]map[string]interface{} - that is then unmarshalled to either
toolboxImage or toolboxContainer. This is done to prevent a major
refactoring before the 0.1.0 release. This should be changed for the
0.2.0 release.
containers#503
Since Podman 2.1 the field 'Created' of `podman images --format json` no longer holds a string with human-readable string in format "5 minutes ago" but holds a Unix time integer[0]. [0] containers/podman#6815 containers#503
HarryMichal
force-pushed
the
get-listing-strong
branch
from
2fcb635 to
f0fd5df
Compare
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
This package will be used for generating a human-readable representation of elapsed time (e.g., "5 minutes"). containers#503
Every time Podman changes their JSON API Toolbox breaks horribly. That
is caused by the combination of decoding the JSON purely by hand and by
the complete lack of type assertions to make the process stable.
I previously didn't know that unmarshalling with Go works on the 'do the
best job it can' logic (thank you Owen!). This makes the need to check
for subtle changes in the names of fields go away and type checking a
bit more bareable (marking the questioned field as interface{} and then
immediatelly type switch).
If even now an existing field does not hold an expected value the field
will remain blank.
To take this a bit further, I created two types (toolboxImage and
toolboxContainer) that are used by both functions for returning the list
of images and containers. Instead of using getters for properties that
need some checks or adjustments this uses a custom unmarshaling function
(Owen's idea; thanks!).
The work around populating the two new types is a bit hacky (marshalling
and already unmarshalled JSON - that was unmarshalled to
[]map[string]interface{} - that is then unmarshalled to either
toolboxImage or toolboxContainer. This is done to prevent a major
refactoring before the 0.1.0 release. This should be changed for the
0.2.0 release.
containers#503
Since Podman 2.1 the field 'Created' of `podman images --format json` no longer holds a string with human-readable string in format "5 minutes ago" but holds a Unix time integer[0]. [0] containers/podman#6815 containers#503
HarryMichal
force-pushed
the
get-listing-strong
branch
from
f0fd5df to
8d4fc75
Compare
|
Build failed.
|
This package will be used for generating a human-readable representation of elapsed time (e.g., "5 minutes"). containers#503
Every time Podman changes their JSON API Toolbox breaks horribly. That
is caused by the combination of decoding the JSON purely by hand and by
the complete lack of type assertions to make the process stable.
I previously didn't know that unmarshalling with Go works on the 'do
the best job it can' logic (thank you Owen!). This makes the need to
check for subtle changes in the names of fields go away and type
checking a bit more bearable (marking the questioned field as
interface{} and then immediatelly type switch).
If even now an existing field does not hold an expected value the field
will remain blank.
To take this a bit further, I created two types (toolboxImage and
toolboxContainer) that are used by both functions for returning the
list of images and containers. Instead of using getters for properties
that need some checks or adjustments this uses a custom unmarshaling
function (Owen's idea; thanks!).
The work around populating the two new types is a bit hacky
(marshalling and already unmarshalled JSON - that was unmarshalled to
[]map[string]interface{} - that is then unmarshalled to either
toolboxImage or toolboxContainer. This is done to prevent a major
refactoring before the 0.1.0 release. This should be changed for the
0.2.0 release.
containers#503
Since Podman 2.1 the field 'Created' of `podman images --format json` no longer holds a string with human-readable string but holds a Unix time integer[0]. [0] containers/podman#6815 containers#503
debarshiray
force-pushed
the
get-listing-strong
branch
from
8d4fc75 to
8168e0f
Compare
|
Thanks for all your work on this, @HarryMichal |
Every time Podman changes their JSON API Toolbox breaks horribly. That
is caused by the complete lack of type guards. With this, the verbosity
of the code goes up a bit but it should be much more resistant to API
changes.
If a field is of a not-expected type, the field is set to "" and
an error message is printed after the rest of the table is printed into
the console.
An alternative to this solution is to unmarshal the JSON response to a
strongly typed structure, as pointed out by @owtaylor.
Closes: #502