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

imageverifier plugin CLI Arguments format issue #10114 #10153

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Anish-M
Copy link

@Anish-M Anish-M commented Apr 30, 2024

Small fixes to the issue 10114
Signed by Anish Mankal, anish.mankal@utexas.edu

@k8s-ci-robot
Copy link

Hi @Anish-M. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@dmcgowan
Copy link
Member

A few things to consider here. First is the current implementation matches the documentation. Any change to the implementation should change the documentation which is the defacto specification for these plugins. This would mean analyzing which plugins have been already started using this or planning on using this and making sure those projects are aware of the change and updated. Secondly, this is not a user facing CLI interface, there is no need for long form and short form of the parameters as there is no expectation a user will manually enter them. We use the single dash form already for other binary plugins such as the shim. It likely comes down to opinion whether single dash is better for plugin binaries and double dash is better for user facing binaries. We should only make a change like this for a technical reason rather than to change one opinion for another.

@Anish-M
Copy link
Author

Anish-M commented May 1, 2024

@dmcgowan Thanks for the info! Regarding your second point, I made those changes to address this issue: #10114. Would it be better to create it such that both options should be allowed? (i.e. a single dash and a double dash option). This would not affect existing projects using the plugin then. I can fix the documentation if needed as well.

@samuelkarp
Copy link
Member

Considering this code is how containerd calls the image verifier CLI implementations, a double-dash would be a breaking change. Any mechanism where both single- and double-dash are implemented on the caller side (e.g., probing or a config option inside containerd's config file) seems more complication than it is worth. A CLI can certainly decide to accept both, but containerd should not switch from a single- to double-dash when invoking the CLI.

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

Successfully merging this pull request may close these issues.

4 participants