Skip to content
This repository has been archived by the owner on Nov 7, 2024. It is now read-only.

cli: Expose certificate_directory #180

Merged
merged 3 commits into from
Dec 9, 2021
Merged

Conversation

mkenigs
Copy link
Contributor

@mkenigs mkenigs commented Dec 9, 2021

Exposes the corresponding options from containers-image-proxy and
skopeo

Also changes authfile type from String to PathBuf for consistency

Helps #121
Depends containers/containers-image-proxy-rs#22

Exposes the corresponding options from containers-image-proxy and
skopeo

Also changes authfile type from String to PathBuf for consistency

Helps ostreedev#121
Depends containers/containers-image-proxy-rs#22
@mkenigs
Copy link
Contributor Author

mkenigs commented Dec 9, 2021

@cgwalters are we going to have defaults/a config file for cert_dir? If so, what? What about authfile?

How do I test this with rpm-ostree? I've patched the build. Or do I just test ostree-ext-cli container image pull inside a cluster?


#[structopt(long)]
/// Directory with certificates (*.crt, *.cert, *.key) used to connect to registry
/// Equivalent to `skopeo --cert-dir`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this too long for the help text?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I just tried this out and it looks like structopt eats newlines, but does do auto-wrapping. I just tried e.g.:

diff --git a/lib/src/cli.rs b/lib/src/cli.rs
index e6cbaee..9726fcb 100644
--- a/lib/src/cli.rs
+++ b/lib/src/cli.rs
@@ -162,6 +162,7 @@ enum ContainerImageOpts {
     },
 
     /// Copy a pulled container image from one repo to another.
+    /// Blah blah.  Lorem ipsum foo bar baz. Lorem ipsum foo bar baz. Lorem ipsum foo bar baz. Lorem ipsum foo bar baz.
     Copy {
         /// Path to the source repository
         #[structopt(long)]

And I get:

walters@toolbox /v/s/w/s/g/o/ostree-rs-ext (main)> cargo b; and ./target/debug/ostree-ext-cli container image --help
   Compiling ostree-ext v0.5.1 (/var/srv/walters/src/github/ostreedev/ostree-rs-ext/lib)
   Compiling ostree-ext-cli v0.1.4 (/var/srv/walters/src/github/ostreedev/ostree-rs-ext/cli)
    Finished dev [optimized + debuginfo] target(s) in 6.94s
ostree-ext-cli-container-image 0.5.1
Commands for working with (possibly layered, non-encapsulated) container images

USAGE:
    ostree-ext-cli container image <SUBCOMMAND>

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

SUBCOMMANDS:
    copy      Copy a pulled container image from one repo to another. Blah blah.  Lorem ipsum foo bar baz. Lorem
              ipsum foo bar baz. Lorem ipsum foo bar baz. Lorem ipsum foo bar baz
    deploy    Perform initial deployment for a container image
    help      Prints this message or the help of the given subcommand(s)
    list      List container images
    pull      Pull (or update) a container image
walters@toolbox /v/s/w/s/g/o/ostree-rs-ext (main)> 

Soo...it should be fine if you just add a period . at the end of sentences. Or it could be:

/// Directory with certificates (*.crt, *.cert, *.key) used to connect to registry; equivalent to `skopeo --cert-dir`.

or so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never added the period but not sure that's important

@cgwalters
Copy link
Member

@cgwalters are we going to have defaults/a config file for cert_dir? If so, what? What about authfile?

I think we should. There's some bike-shedding involved for that. My strawman would be supporting files in /etc/ostree. On detail: I think we should support these being symbolic links, so that e.g. for OpenShift/MCO we can just ln -s /var/lib/kubelet/config.json /etc/ostree/container-authfile.json or so.

(Now the kubelet pull secret should really be in /etc; that's openshift/machine-config-operator#1190 (comment) )

@cgwalters
Copy link
Member

@lucab there's a crate for systemd-style config right? I vaguely recall something in coreos using it; tried looking in afterburn but didn't see it.

@cgwalters
Copy link
Member

How do I test this with rpm-ostree? I've patched the build. Or do I just test ostree-ext-cli container image pull inside a cluster?

Yep, the latter should work fine! One trick here I've used is oc cp to copy binaries built from my local workstation to a remote cluster.

@cgwalters
Copy link
Member

OK so to make the CI here green, we need to change our Cargo.toml to pull from git. This is another variant of https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html#the-patch-section

Do you want to do that or OK if I just push the fixups for this?

@cgwalters
Copy link
Member

@cgwalters cgwalters enabled auto-merge December 9, 2021 21:30
@cgwalters cgwalters merged commit fd6dc4c into ostreedev:main Dec 9, 2021
cgwalters added a commit to cgwalters/containers-image-proxy-rs that referenced this pull request Dec 9, 2021
Now that ostreedev/ostree-rs-ext#180
landed.  This completes the "ratcheting" we needed to do to land both
PRs.
@mkenigs
Copy link
Contributor Author

mkenigs commented Dec 21, 2021

@lucab there's a crate for systemd-style config right? I vaguely recall something in coreos using it; tried looking in afterburn but didn't see it.

@lucab did you ever see that?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants