-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Adding dial-stdio CLI cmd #11819
Adding dial-stdio CLI cmd #11819
Conversation
You have to add tests, if possible otherwise add [no tests needed] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmd/podman/system/dial_stdio.go
Outdated
ctx := registry.Context() | ||
ctx, cancel := context.WithCancel(ctx) | ||
defer cancel() | ||
bindCtx, err := bindings.NewConnection(ctx, "unix:///var/run/docker.sock") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this configurable at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should default to same as the podman remote connection, so unix:///run/podman/podman.sock
and unix:///run/user/<UID>/podman/podman.sock
as rootless
if they run a podman command we should talk to the podman socket and not docker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I've updated it to grab the URI from the config which handles it as you say (unix:///run/podman/podman.sock for root, unix:///run/user//podman/podman.sock for rootless by default)
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan, trynaeat 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 |
LGTM |
All kinds of test unhappiness @trynaeat |
3bee687
to
2b84e6f
Compare
Fixed the failing integration tests on my part. Any insight into the last failing CI job, @rhatdan ? Some kind of error from the package manager before it even gets to the test job. |
cmd/podman/system/dial_stdio.go
Outdated
} | ||
|
||
// Below portion taken from original docker CLI | ||
// https://github.com/docker/cli/blob/master/cli/command/system/dial_stdio.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to use permalink
i.e., s/master/v20.10.9/
You have to rebase to the latest main branch to fix this test. |
Adding dial-stdio CLI cmd Signed-off-by: Jake Parks <[email protected]> Made dial-stdio URI configurable Slight refactors Signed-off-by: Jake Parks <[email protected]> Added simple test for existence of `podman system dial-stdio` command Fix 'system dial-stdio' integration tests Changed link in comment to permalink
/lgtm |
Signed-off-by: Jake Parks [email protected]
What this PR does / why we need it:
Fixes #11668
How to verify it
Which issue(s) this PR fixes:
Fixes #11668
Special notes for your reviewer: