-
-
Notifications
You must be signed in to change notification settings - Fork 97
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
Parsing changes for podman support #303
base: master
Are you sure you want to change the base?
Conversation
This includes a failing test for a result observed with podman
The only place this is used (.WaitForHealthy()) already performs a null- safe check against the Status, comparing it to "Healthy" only. Changing the property to nullable allows more defensive parsing if it is an empty string for whatever reason, and is still interpreted as "Unhealthy" where it matters.
This adds and uses a custom converter which will parse Container.Config.Entrypoint from a json string array I.E. ```json { "EntryPoint": [ "my-entrypoint" ] } ``` Or from a single string value, I.E: ```json { "EntryPoint": "my-entrypoint" } ```
In some cases the `--format` specification doesn't match the output resulting in an error such as: `Error: template: ls:1:43: executing "ls" at <.Scope>: can't evaluate field Scope in type network.ListPrintReports"` If the full command is unsuccessful, this falls back to parsing just the ID and Name, which prevents crashes at the cost of a less rich experience.
This introduces a slightly more complex parsing method for timespans in the ProcessRow class in order to deal with more diverse formats (such as the format output by `podman top` - 1h2m3s vs 01:02:03). This also adds a simple test to validate the parser's current behaviour with docker, and a test to confirm it can parse podman output too without crashing.
Hi @Rory-Reid and many thanks for this excellent PR!! It seems to fail in two tests, I currently do not have the time to check why? Could you have a quick check and see if it is the current implementation affecting it? Cheers, |
Hi @mariotoffia and firstly thanks for looking at my PR! I appreciate it's not a small one! Ah, yeah, this is the same two tests that are failing on the master branch as far as I can see - Both of these passed locally for me so I assumed it was environmental and wasn't familiar with the CI setup but on rerun I can see they pass with an error in my logs because I don't have Given they're currently failing on master I don't think they're related to my changes but I was purposefully leaving this PR as a draft until I could rebase onto a working build to prove that. I'll see if I can prove anything locally at least around this sometime soon. |
This is a summary of issues I have worked on in order to get closer to an initial pass for supporting podman (#300)
I've tried to do them as podman-agnostic as possible (more general defensive approaches to parsing slightly different formats of things) but it's a non-zero amount of complexity added now for this. I understand if that's not something you'd be interested in bringing into the library.