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

Add support for podman context as alias to podman system connection #15072

Merged
merged 1 commit into from
Sep 19, 2022

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Jul 26, 2022

Alias
podman context use -> podman system connection default
podman context rm -> podman system connection rm
podman context create -> podman system connection add
podman context ls ->podman system connection ls

Podman context is a hidden command, but can be used for existing scripts
that assume Docker under the covers.

Signed-off-by: Daniel J Walsh [email protected]

Does this PR introduce a user-facing change?

podman context command is a hidden command available for Docker compatibility.

@openshift-ci openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 26, 2022
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.

I don't think it makes sense to just alias the names. If we need docker compat we have to actually match the docker output and input (cli arguments).

@vrothberg
Copy link
Member

I don't think it makes sense to just alias the names. If we need docker compat we have to actually match the docker output and input (cli arguments).

Agreed. Let's discuss which goal we try to reach. If it's docker compat, we probably need to tackle it command-by-command/commit-by-commit to make it reviewable.

@rhatdan
Copy link
Member Author

rhatdan commented Jul 26, 2022

Yup, I agree, although don't have time or understanding of how these match up. I was hoping to get this to pass tests and ask people who are hitting issues, to see if this gets them further. Then if someone (intern?) wants to take it over and drive it home for missing CLI and Output.

I have a feeling getting use and list to work would get us 95% of the way towards making more tools work.

@rhatdan rhatdan force-pushed the context branch 2 times, most recently from 8b16378 to ed47f25 Compare July 27, 2022 18:29
@rhatdan rhatdan changed the title Add support for podman context as alias to podman system connection [wip] Add support for podman context as alias to podman system connection Jul 30, 2022
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 30, 2022
@rhatdan rhatdan force-pushed the context branch 5 times, most recently from 88f92dd to 5ebe0b9 Compare August 2, 2022 14:27
@rhatdan rhatdan changed the title [wip] Add support for podman context as alias to podman system connection Add support for podman context as alias to podman system connection Aug 2, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 2, 2022
@rhatdan rhatdan force-pushed the context branch 5 times, most recently from 9146db7 to dda3ea2 Compare August 16, 2022 15:00
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 10, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 10, 2022
Alias
podman --context -> podman --connection
podman context use -> podman system connection default
podman context rm -> podman system connection rm
podman context create -> podman system connection add
podman context ls ->podman system connection ls
podman context inspect ->podman system connection ls --json (For
specified connections)

Podman context is a hidden command, but can be used for existing scripts
that assume Docker under the covers.

Signed-off-by: Daniel J Walsh <[email protected]>
@rhatdan
Copy link
Member Author

rhatdan commented Sep 15, 2022

@containers/podman-maintainers PTAL
This one is finally ready to merge.
@vrothberg @mheon @giuseppe @Luap99 @baude PTAL

@@ -56,8 +56,22 @@ function _run_podman_remote() {
c1="c1_$(random_string 15)"
c2="c2_$(random_string 15)"

run_podman system connection add $c1 tcp://localhost:12345
run_podman system connection add --default $c2 tcp://localhost:54321
Copy link
Member

Choose a reason for hiding this comment

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

I'm slightly nervous about removing tests for system connection add --default. I realize the code paths are the same, but the option parsing itself is now untested. If for some reason you need to resubmit, or if you too think this path should be tested, I think an easy check would be to add

run_podman system connection add --default $c3 tcp://localhost:something   <--- be sure to define c3
run_podman system connection ls
is "$output" "...check that $c3 is now default, left as exercise for the reader"

...to the tail end, after you do all the c1/c2 checks.

But maybe this is not a valid concern.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it is worth it, but I would rather get this merged and then open a PR to add more tests.

Copy link
Member

Choose a reason for hiding this comment

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

That's fine with me.

@rhatdan
Copy link
Member Author

rhatdan commented Sep 19, 2022

@cdoern @vrothberg @giuseppe @flouthoc PTAL
@containers/podman-maintainers PTAL

Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@giuseppe giuseppe 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 openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 19, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 19, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, giuseppe, rhatdan

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:
  • OWNERS [flouthoc,giuseppe,rhatdan]

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

@openshift-merge-robot openshift-merge-robot merged commit 5f5d400 into containers:main Sep 19, 2022
@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 Sep 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
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.

7 participants