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

Remove the extra (defaults to true) help msg #1431

Merged
merged 1 commit into from
Sep 25, 2021

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Aug 26, 2021

By default skopeo checks to see if the user actually uses one of the
--*tls-verify flags. Their initial value is ignored. Setting the
initial value to false causes Cobra to not display the default value on
the screen when the user runs a skopeo --help command.

If the user does not specify a --*tls-verify option, it falls back to
using the value specified in the registries.conf file.

Fixes: #1383

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

@TomSweeneyRedHat
Copy link
Member

Couple nits for consideration

Copy link
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

I’m afraid this is just incorrect; optionalBoolFlag’s default is not (present=true, value=true), it is (present=false, value=N/A).

In particular --tls-verify=true overrides an insecure field in registries.conf, but missing --tls-verify=true uses the insecure field’s value.

Copy link
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

false is just as invalid, for the same reason — it overrides registries.conf, whereas the default doesn’t.

@mtrmac
Copy link
Contributor

mtrmac commented Aug 26, 2021

false is just as invalid, for the same reason — it overrides registries.conf, whereas the default doesn’t.

… actually, as long as it doesn’t show up in any user-visible text, nor in any user-visible behavior, that might be fine. But that needs a somewhat comprehensive argument (and given the brittleness, a written record in the commit message or PR) of why that is the case.

By default skopeo checks to see if the user actually uses one of the
--*tls-verify flags. Their initial value is ignored.  Setting the
initial value to false causes Cobra to not display the default value on
the screen when the user runs a `skopeo --help` command.

If the user does not specify a --*tls-verify option, it falls back to
using the value specified in the registries.conf file.

Fixes: containers#1383

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
@rhatdan
Copy link
Member Author

rhatdan commented Aug 27, 2021

@mtrmac commit message good now?

@rhatdan
Copy link
Member Author

rhatdan commented Aug 31, 2021

@mtrmac @vrothberg PTAL

Copy link
Member

@lsm5 lsm5 left a comment

Choose a reason for hiding this comment

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

LGTM, maybe needs a rebase or could be a github problem.

@rhatdan Nit RE: commit verification. It says Unverified against your commit probably because you haven't uploaded your GPG to github.

@rhatdan rhatdan merged commit 2c2e5b7 into containers:main Sep 25, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Help of --tls-verify has (default twice
5 participants