provide suggestion if xpc 'Connection invalid' error encountered

[heywoodlh]

Closes #80

Adds the following help message if you try to run container against a host that hasn't started the container system:

❯ /usr/local/bin/container list
Error: internalError: "failed to list containers" (cause: "interrupted: "Connection invalid: ensure container system has been started with `container system start`"")

❯ /usr/local/bin/container run -it --rm docker.io/alpine
Error: interrupted: "Connection invalid: ensure container system has been started with `container system start`"

[adityaramani]

Thanks for the PR! Im trying to think about this a little bit.

I think I'd like it better if this check happens somewhere in the CLI - im not sure where the right place to put it in is (in a way that we dont need to update every command). Maybe ArgumentParser - the library we use to write the CLI has an exit hook?

The reason for this is that XPCClient in theory is a (mostly) generic XPC client type and could be used for other services that may uses a different bootstrap pattern.

What are your thoughts on this?

[heywoodlh]

My thought was to emulate a similar UX to the Docker client since it's the dominant container platform as highlighted here: #80 (comment)

Honestly, I'm not super concerned about where it's at. I think it makes sense to have on every command where the XPC client fails to connect -- as that's a fatal error.

Regardless, I'm happy to rework it however is most desired -- let me know what you'd prefer and I'll do it. 😄

[adityaramani]

Yes totally agree about having the check there - just that I think its better if its somewhere in the CLI. Let me look a bit more to see if there is an easy way to add it.

This will also allow others to share their thoughts

[heywoodlh]

I'll spend some time reworking it into the CLI as you suggested and see what I can come up with. Likely will get around to it tomorrow! 😄

[adityaramani]

Went through the code a bit - I think we can perform that check here? https://github.com/apple/container/blob/main/Sources/CLI/Application.swift#L179

And it should account for all the commands we have?

[heywoodlh]

Latest iteration of my PR, the error-handling looks like this:

❯ container system stop

❯ ./bin/container list
Warning! Running debug build. Performance may be degraded.
Error: internalError: "failed to list containers" (cause: "interrupted: "XPC connection error: Connection invalid"")
Ensure container system has been started with `container system start`.

❯ ./bin/container run -it --rm docker.io/alpine:latest
Warning! Running debug build. Performance may be degraded.
Error: interrupted: "XPC connection error: Connection invalid"
Ensure container system has been started with `container system start`.

Please let me know if you have any further feedback on wording, etc. 😄 Sorry for taking a while to get around to this!

[adityaramani]

Thanks! I like this better!

Its unfortunate that we need to use String(describing: error) and check the string - but I dont think there is a better way.

We cant solely use the error codes / cause of the error in ContainerizationError cause they may not give a match all the time

[adityaramani]

Can we switch this to be ContainerizationError over NSError? Or Did you see that using NSError was causing the exit() to print the message better?

[heywoodlh]

Done in latest rev!

[heywoodlh]

However, is there a way with a ContainerizationError to remove the quotes at the beginning and end of the error output?

❯ ./bin/container list
Warning! Running debug build. Performance may be degraded.
Error: internalError: "internalError: "failed to list containers" (cause: "interrupted: "XPC connection error: Connection invalid"")
Ensure container system service has been started with `container system start`."

[heywoodlh]

Also, noticed in my output that internalError is repeated twice. Is there a more generic ContainerizationError type that is not .internalError that I could use?

[adityaramani]

We do have .interrupted which may make sense? or .invalidState ?
I need to check if we can do something about the quotes

[heywoodlh]

Interrupted looks good to me!

❯ ./bin/container list
Warning! Running debug build. Performance may be degraded.
Error: interrupted: "internalError: "failed to list containers" (cause: "interrupted: "XPC connection error: Connection invalid"")
Ensure container system service has been started with `container system start`."

[adityaramani]

Thanks! LGTM! Will wait for the others to look if we want to change the wordings.

You just need to run make fmt and re-push

[heywoodlh]

@katiewasnothere I forgot to run make fmt before pushing! That has been fixed on latest rev. 😄