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

phase 1, migrate the outer shell of cosign to cobra #728

Merged
merged 1 commit into from
Sep 20, 2021
Merged

phase 1, migrate the outer shell of cosign to cobra #728

merged 1 commit into from
Sep 20, 2021

Conversation

n3wscott
Copy link
Contributor

Summary

This is the first in several PRs to migrate cosign to cobra. In this PR I have focused on:

  • Adding deprecation warnings for using non-POSIX compliant flags.
  • grouping subcommands together where needed to prevent circular dependencies.
  • moving core non-cli logic out of cmd/ and into pkg/ where needed to prevent circular dependencies.

Ticket Link

Related to #706

Release Note

- Deprecation Warning: cosign is moving to POSIX style flags. All flags that are single dash and a word are deprecated in favor of double dash and word (`-foo` to `--foo`) and double dash single letter flags are deprecated in favor of single dash flags (`--f` to `-f`).

@n3wscott n3wscott force-pushed the migrate-cosign branch 3 times, most recently from 433a53f to 87d1c20 Compare September 20, 2021 18:19
dekkagaijin
dekkagaijin reviewed Sep 20, 2021
View reviewed changes
cmd/cosign/cli/options/root.go
cmd.PersistentFlags().StringVar(&o.OutputFile, "output-file", "",
"log output to a file")

cmd.PersistentFlags().BoolVarP(&o.Verbose, "verbose", "d", false,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cmd.PersistentFlags().BoolVarP(&o.Verbose, "verbose", "d", false,
cmd.PersistentFlags().BoolVarP(&o.Verbose, "verbose", "v", false,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the current option is d for debug, but debug is not a normal flag, verbose is :idk:

cmd/cosign/cli/options/root.go
}

func AddRootArgs(cmd *cobra.Command, o *RootOptions) {
cmd.PersistentFlags().StringVar(&o.OutputFile, "output-file", "",
Copy link
Member

Choose a reason for hiding this comment

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

--log-out? --log-file?

We'll probably be wanting to use --output-file and/or -o for the commands that rewrite dockerfiles and k8s manifests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to change the flag, but yeah I like that better. Let's get the refactor in place and revisit what we want long term

cmd/cosign/cli/sign/sign_blob.go
Comment on lines +83 to 88
if !options.EnableExperimental() {
if !options.OneOf(*key, *sk) {
return &options.KeyParseError{}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if !options.EnableExperimental() {
if !options.OneOf(*key, *sk) {
return &options.KeyParseError{}
}
}
if !options.EnableExperimental() && !options.OneOf(*key, *sk) {
return &options.KeyParseError{}
}

cmd/cosign/cli/verify/verify_blob.go
@@ -114,19 +118,19 @@ func isb64(data []byte) bool {
return err == nil
}

func VerifyBlobCmd(ctx context.Context, ko KeyOpts, certRef, sigRef, blobRef string) error {
var pubKey signature.Verifier
func VerifyBlobCmd(ctx context.Context, ko sign.KeyOpts, certRef, sigRef, blobRef string) error {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func VerifyBlobCmd(ctx context.Context, ko sign.KeyOpts, certRef, sigRef, blobRef string) error {
func Blob(ctx context.Context, ko sign.KeyOpts, certRef, sigRef, blobRef string) error {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:D I started doing this but all of the files are not constant. the plan is to move all of this code into another form, so this whole thing is throw away anyway soon. let's hold off renaming at this moment, I added a // nolint comment for now.

Copy link
Member

Choose a reason for hiding this comment

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

ty

cmd/cosign/cli/verify/verify_blob.go
"github.com/sigstore/sigstore/pkg/signature"
"github.com/sigstore/sigstore/pkg/signature/options"
sigstoresigs "github.com/sigstore/sigstore/pkg/signature"
signatureoptions "github.com/sigstore/sigstore/pkg/signature/options"
)

func VerifyBlob() *ffcli.Command {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func VerifyBlob() *ffcli.Command {
func BlobCmd() *ffcli.Command {

cmd/cosign/cli/verify/verify.go
"github.com/sigstore/cosign/pkg/cosign"
"github.com/sigstore/cosign/pkg/cosign/pivkey"
"github.com/sigstore/cosign/pkg/image"
sigs "github.com/sigstore/cosign/pkg/signature"
"github.com/sigstore/sigstore/pkg/signature"
"github.com/sigstore/sigstore/pkg/signature/payload"
)

// VerifyCommand verifies a signature on a supplied container image
type VerifyCommand struct {
Copy link
Member

Choose a reason for hiding this comment

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

This one's weirder than the other surfaces, but would be good to address the linter feedback here, too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, agreed, but not in this PR?

Copy link
Member

Choose a reason for hiding this comment

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

sgtm

dekkagaijin
dekkagaijin approved these changes Sep 20, 2021
View reviewed changes
Copy link
Member

@dekkagaijin dekkagaijin left a comment

Choose a reason for hiding this comment

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

approved w/ comments for further iteration

@dekkagaijin dekkagaijin merged commit 5fb178c into sigstore:main Sep 20, 2021
@github-actions github-actions bot added this to the v1.3.0 milestone Sep 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dekkagaijin dekkagaijin dekkagaijin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.3.0
Development

Successfully merging this pull request may close these issues.
2 participants
@n3wscott @dekkagaijin