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.

Sign up for GitHub

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

Jump to bottom

Refactor podman system connection #6938

Merged
merged 1 commit into from
Jul 24, 2020
Merged

Refactor podman system connection #6938

merged 1 commit into from
Jul 24, 2020

Conversation

jwhonce
Copy link
Member

@jwhonce jwhonce commented Jul 10, 2020

  • Add support to manage multiple connections
    • Add connection
    • Remove connection
    • Rename connection
    • Set connection as default

Signed-off-by: Jhon Honce [email protected]

@jwhonce jwhonce added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 10, 2020
@jwhonce jwhonce requested a review from baude July 10, 2020 23:21
@jwhonce jwhonce self-assigned this Jul 10, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 10, 2020
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jwhonce

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 10, 2020
@baude
Copy link
Member

baude commented Jul 13, 2020

@rhatdan wdyt?

@rhatdan
Copy link
Member

rhatdan commented Jul 13, 2020

I don't see the reason for rename, I would just tell them to edit the file. Or Remove and re-add.

@jwhonce jwhonce force-pushed the wip/n-connection branch from 1699d6d to 42934a8 Compare July 14, 2020 18:56
@jwhonce jwhonce mentioned this pull request Jul 16, 2020
Merged
@jwhonce jwhonce force-pushed the wip/n-connection branch from 42934a8 to a505c38 Compare July 16, 2020 23:50
@jwhonce
Copy link
Member Author

jwhonce commented Jul 16, 2020

Blocked on #7000

@rhatdan
Copy link
Member

rhatdan commented Jul 17, 2020

#7000 merged.

@jwhonce jwhonce force-pushed the wip/n-connection branch from a505c38 to 7fa8c87 Compare July 17, 2020 21:08
@jwhonce jwhonce changed the title [WIP] Refactor podman system connection Refactor podman system connection Jul 17, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 17, 2020
@jwhonce jwhonce force-pushed the wip/n-connection branch 5 times, most recently from d9b8729 to 1c95f1b Compare July 20, 2020 22:50
@edsantiago
Copy link
Member

Ouch - I see the reason for the test failure now. As it is now, this command breaks precedent because it is the only podman something subcommand that does not require an argument. All the others -- image, container, pod, network, whatever -- can never be invoked standalone, they all require a sub-subcommand.

Super-unfortunately, this is nonintuitive because the --help message implies that it's optionsl:

$ podman something --help
Usage:
  podman something [command]

One way around this is to special-case the .bats test so it recognizes and skips podman system connection.

Another way, which I lean toward, is to add ls so podman system connection ls will list connections, and with no args it will barf. This preserves consistency with every other podman subcommand; I also think it's a tad more intuitive. But it's your call.

@rhatdan
Copy link
Member

rhatdan commented Jul 21, 2020

I like the idea of ls

@jwhonce jwhonce force-pushed the wip/n-connection branch 5 times, most recently from cf30df2 to be827fe Compare July 22, 2020 17:28
@jwhonce
[WIP] Refactor podman system connection
964d330 
* Add support to manage multiple connections
  * Add connection
  * Remove connection
  * Rename connection
  * Set connection as default
  * Add markdown/man pages
* Fix recursion in hack/xref-helpmsgs-manpages

Signed-off-by: Jhon Honce <[email protected]>
@jwhonce jwhonce force-pushed the wip/n-connection branch from be827fe to 964d330 Compare July 22, 2020 22:26
edsantiago
edsantiago reviewed Jul 23, 2020
View reviewed changes
cmd/podman/root.go
lFlags.BoolVarP(&opts.Remote, "remote", "r", false, "Access remote Podman service (default false)")
lFlags.StringVar(&opts.URI, "url", defaultURI, "URL to access Podman service (CONTAINER_HOST)")
lFlags.StringVar(&opts.Identity, "identity", custom.Engine.RemoteIdentity, "path to SSH identity file, (CONTAINER_SSHKEY)")
lFlags.StringVar(&opts.URI, "url", uri, "URL to access Podman service (CONTAINER_HOST)")
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 feeling pretty squicked out by the interchange of URI and URL. I think that will lead to maintenance headaches. From my (limited) understanding, I believe URL (l as in Lima) is the correct term. Would you consider replacing the code instances of uri (i as India) to url for consistency?

Copy link
Member

Choose a reason for hiding this comment

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

Yes let's stick with URL

Copy link
Member Author

@jwhonce jwhonce Jul 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@rhatdan @edsantiago

uri was chosen for a reason. url is a golang package name. There is a lot of code that uses the url package to process the "uri" variable. We would be forced to alias the package name or make up longer names which I deemed to be a large maintenance headache. I attempt to use uri for internals and URL for user facing so everyone is happy.

As a url is a uri this naming simplifies the coding and is correct naming.

Copy link
Member

Choose a reason for hiding this comment

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

Would calling it path be a good compromise? It's less specific as to what exact type of path it is, and only one character longer.

Copy link
Member Author

@jwhonce jwhonce Jul 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@mheon Path is an element of the url/uri, so that would be confusing to see path.Path.

Copy link
Member

Choose a reason for hiding this comment

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

How about we call everything URI and drop URL, if we want to still support the --url for backwards compatibility just make it an alias.

@rhatdan
Copy link
Member

rhatdan commented Jul 23, 2020

LGTM

@mheon
Copy link
Member

mheon commented Jul 24, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 24, 2020
@openshift-merge-robot openshift-merge-robot merged commit 22b1483 into containers:master Jul 24, 2020
@Luap99 Luap99 mentioned this pull request Jul 25, 2020
Closed
@jwhonce jwhonce deleted the wip/n-connection branch June 30, 2021 16:09
@jwhonce jwhonce restored the wip/n-connection branch June 30, 2021 16:09
@jwhonce jwhonce deleted the wip/n-connection branch June 30, 2021 16:09
@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 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

@mheon mheon mheon left review comments

@rhatdan rhatdan rhatdan left review comments

@edsantiago edsantiago edsantiago left review comments

@baude baude Awaiting requested review from baude

@umohnani8 umohnani8 Awaiting requested review from umohnani8

Assignees

@jwhonce jwhonce

@mheon mheon

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@jwhonce @openshift-ci-robot @baude @rhatdan @edsantiago @mheon @TomSweeneyRedHat @openshift-merge-robot