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

Add Sigstore sign/verify to CLI #310

Merged
merged 8 commits into from
Oct 11, 2024
Merged

Conversation

font
Copy link
Member

@font font commented Oct 3, 2024

Summary

  • Update to support Sigstore sign via CLI
  • Update to support Sigstore verify via CLI
  • Update docs for Sigstore sign/verify CLI
  • Update doc to reflect latest CLI changes

Release Note

Documentation

@font font requested review from a team as code owners October 3, 2024 01:19
Copy link
Collaborator

@mihaimaruseac mihaimaruseac left a comment

Choose a reason for hiding this comment

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

I like this. Two more things to do:

  • can we make sigstore signing method be the default?
  • can you fix the failing lint CI?

@font font force-pushed the sigstore-oidc branch 2 times, most recently from 09596b6 to 5db29ad Compare October 8, 2024 00:37
Signed-off-by: Ivan Font <[email protected]>
@font
Copy link
Member Author

font commented Oct 8, 2024

@mihaimaruseac Thanks for the review!

  • can we make sigstore signing method be the default?

I don't think we can do this as there doesn't appear to be a way to set a default subparser, and if no method is specified on the command line, that parser's Namespace object does not contain the option attributes defined for it. I may have to look more into seeing if there's a way to do this. Let me know if you have any suggestions.

  • can you fix the failing lint CI?

Done

Copy link
Member Author

@font font left a comment

Choose a reason for hiding this comment

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

@mihaimaruseac This is ready for re-review.

@mihaimaruseac
Copy link
Collaborator

I don't think we can do this as there doesn't appear to be a way to set a default subparser, and if no method is specified on the command line, that parser's Namespace object does not contain the option attributes defined for it. I may have to look more into seeing if there's a way to do this. Let me know if you have any suggestions.

We can use set_defaults. I did something similar in the benchmarks:

parser.set_defaults(func=generate_file)

@font
Copy link
Member Author

font commented Oct 9, 2024

We can use set_defaults. I did something similar in the benchmarks:

parser.set_defaults(func=generate_file)

Thanks @mihaimaruseac!

I had looked at the set_defaults approach and even tried it, but it seems it's mostly a good way to organize different functionality into its own code paths.

That is, the generate.py code runs into the same problem. Namely that when there's options that are part of a positional argument, those options never get added to that positional argument, or parser's, Namespace object. Here's the specific error:

→ python generate.py 
Traceback (most recent call last):
  File "model-transparency/benchmarks/generate.py", line 227, in <module>
    args.func(args)
  File "model-transparency/benchmarks/generate.py", line 81, in generate_file
    create_file_of_given_size(args.root, args.size)
                              ^^^^^^^^^
AttributeError: 'Namespace' object has no attribute 'root'

So I'm not sure there's a good way to set a default for a required positional argument that also has its own options unless we just start manipulating the Namespace object directly. But that feels a little kludgy. Thoughts?

@mihaimaruseac
Copy link
Collaborator

Oh, I only tested with -h :(

In this case, let's merge this as it is and then move towards merging the sign and verify CLI scripts into a single one, as then we could make the signer be set via a flag, with defaults.

@mihaimaruseac mihaimaruseac merged commit 1ebab2f into sigstore:main Oct 11, 2024
18 checks passed
@mihaimaruseac mihaimaruseac added this to the V1 release milestone Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants