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

Parsing changes for podman support #303

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Rory-Reid
Copy link

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.

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.
@Rory-Reid Rory-Reid mentioned this pull request Sep 29, 2023
@mariotoffia
Copy link
Owner

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,
Mario :)

@Rory-Reid
Copy link
Author

Rory-Reid commented Nov 5, 2023

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 - PullContainerBeforeRunningShallWork and ContainerHealthCheckShallWork.

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 docker-machine ("Failed to find docker-machine client binary - please add it to your path").

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.

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.

2 participants