Add skopeo Login from c/common

[QiWang19]

Used c/common/pkg/auth for skopeo login
Based on PR #857

[QiWang19]

@rhatdan PTAL
This PR is on top of #857, but it's still under review.

[mtrmac]

What is the ultimate dependency graph envisioned to look like? This somewhat suggests Podman → CLI parsing + half of the implementation in Skopeo → another half of the implementation in Buildah.

And where does the actual Buildah CLI fit into this? It can’t call Skopeo, that would be a circular dependency.

[rhatdan]

Lets move the shared code to github.com/containers/common, And not vendor buildah into skopeo.

[QiWang19]

How much code should be shared i c/common? If skopeo keeps OptionalBool, podman and buildah CLI cannot fully fit in skopeo. They can share a subset, and skopeo should defined its own cli for Optional.. flags.

[mtrmac]

(This should have been the first design concern before writing code I think.)

We can’t trivially share global code anyway, due to things like --debug vs --log-level=debug. (Well, we could work to make them all the same. But that’s non-trivial and a fair bit of work.)

My first guess would be, in Skopeo terms, to make the loginOptions, and loginOptions.run public, and let every application configure its CLI parsing. That would make all flags consistent within projects, but different across all the logins, and require a fair bit of code.

Alternatively, make globalOptions and func loginCommand(*globalOptions) *cobra.Command public, let every application configure CLI parsing for the global options, make the login options consistent between projects (but different from other subcommands in that project).

[mtrmac]

BTW making the “optional bool” flag type public and used elsewhere would make sense to me, if we can make it work with Cobra nicely.

[rhatdan]

Well if we need OptionalBool for Skopeo, I do not know why we don't need it for Buidlah And Podman.

[rhatdan]

All of the handling of certs an plugins can be isolated from the CLI Parsing can't it?

[mtrmac]

Well if we need OptionalBool for Skopeo, I do not know why we don't need it for Buidlah And Podman.

optionalBool and forbidding string-based parameter matching was an attempt to learn from #523 and make that kind of vulnerability impossible. Buildah/Podman could simply not make those kind of mistakes, I guess :)

---

All of the handling of certs an plugins can be isolated from the CLI Parsing can't it?

In principle, but that kind of depends on the adjustments that are not “CLI parsing”, e.g. Podman overriding the defaults in “rootless” mode and creating the directories in advance. Those kinds of activities are not “CLI parsing” but it must be possible to inject them into the operation at some point (or, well, they could be removed and pushed down into the backend libraries, as has been somewhat happening recently).

[QiWang19]

@mtrmac @rhatdan PTAL. The skopeo uses public CLI definition from c/common/auth, keep its own cli parsing newSystemcontext

[rhatdan]

@QiWang19 Needs a rebase.

[QiWang19]

can the tests for login/logout use the registry set up in integration/registry.go?

[mtrmac]

can the tests for login/logout use the registry set up in integration/registry.go?

Sure; check_test.go and other parts already set up several registries that can maybe just be reused.

[QiWang19]

@mtrmac PTAL

[mtrmac]

This should be removed from imageOptions.newSystemContext now (and I guess replaced with a comment that explains that the imageOptions option overrides the global one if both are present.

[mtrmac]

(Non-blocking: I meant a comment specific to --tls-verify. I guess the current generic one works.)

[QiWang19]

@mtrmac PTAL

[mtrmac]

(Non-blocking: I meant a comment specific to --tls-verify. I guess the current generic one works.)

[mtrmac]

Looks good, last nits.

[QiWang19]

@mtrmac PTAL

[mtrmac]

#865 (comment) is still outstanding, and please rebase. Looks good otherwise.

[QiWang19]

@mtrmac PTAL

[mtrmac]

LGTM. Thanks!