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

Show client info even if remote connection fails #20333

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

baude
Copy link
Member

@baude baude commented Oct 11, 2023

When people report issues, we often ask for the result of podman info. However, if the problem is the remote connection, it will error out with no information at all. This PR at least will report client information before disclosing the connection error. For example on Windows:

.\bin\windows\podman.exe info
client:
OS: windows/amd64
provider: hyperv
version: 4.8.0-dev
host: null
...

Satisfies: RUN-1720

Does this PR introduce a user-facing change?

podman info now reports client information even if the remote connection is unreachable

@openshift-ci openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 11, 2023
@edsantiago
Copy link
Member

Nice! Very easy to add a system test, should you want to & should you need to repush:

run_podman 125 --url unix://localhost/no/such/path info
assert "$output" =~ "host: null"
assert "$output" =~ "Error: unable to connect to Podman socket"

@packit-as-a-service
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@lsm5
Copy link
Member

lsm5 commented Oct 11, 2023

Ephemeral COPR build failed. @containers/packit-build please check.

ignore eln

@rhatdan
Copy link
Member

rhatdan commented Oct 11, 2023

LGTM, other then cross compiler issues.

@@ -9,6 +9,7 @@ import (
// running libpod/podman
// swagger:model LibpodInfo
type Info struct {
Client *ClientInfo `json:"client"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a API change and nothing is ever set via API so this makes little sense to expose this over the wire.

I recommend to add a new type on the client side only, something like this in cmd/podman/system/info.go:

type Info struct {
	*define.Info
	Client *ClientInfo `json:"client"`
}

type ClientInfo struct {
	OSArch   string `json:"OS"`
	Provider string `json:"provider"`
	Version  string `json:"version"`
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, fair enough ... i kind of figured that was coming.

@baude
Copy link
Member Author

baude commented Oct 11, 2023

Nice! Very easy to add a system test, should you want to & should you need to repush:

run_podman 125 --url unix://localhost/no/such/path info
assert "$output" =~ "host: null"
assert "$output" =~ "Error: unable to connect to Podman socket"

what file should i add that to?

@edsantiago
Copy link
Member

001-basic or 005-info. The former already has a few tests that use --url (with the requisite skip_if_remote), the latter is possibly more suited toward a test of podman info. In either case I think it could be piggybacked onto an existing test.

@baude
Copy link
Member Author

baude commented Oct 11, 2023

meh, i pushed before i saw your comment; lets see how things go

@edsantiago
Copy link
Member

Suggestion:

--- a/test/system/272-system-connection.bats
+++ b/test/system/272-system-connection.bats
@@ -104,3 +104,3 @@ $c2[ ]\+tcp://localhost:54321[ ]\+true" \
     is "$output" \
-       "Cannot connect to Podman. Please verify.*dial tcp.*connection refused" \
+       "OS: .*provider:.*Cannot connect to Podman. Please verify.*dial tcp.*connection refused" \
        "podman info, without active service"

When people report issues, we often ask for the result of `podman info`.
However, if the problem is the remote connection, it will error out with
no information at all.  This PR at least will report client information
before disclosing the connection error.  For example on Windows:

> .\bin\windows\podman.exe info
client:
  OS: windows/amd64
  provider: hyperv
  version: 4.8.0-dev
  host: null

Satisfies: RUN-1720

Signed-off-by: Brent Baude <[email protected]>
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 13, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude, Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rhatdan
Copy link
Member

rhatdan commented Oct 13, 2023

/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 13, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2023
@edsantiago
Copy link
Member

/hold cancel

Thanks, @baude

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 13, 2023
@openshift-ci openshift-ci bot merged commit b5fec41 into containers:main Oct 13, 2023
98 checks passed
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Jan 12, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants