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

Implement flags over native gem & config handling #19

Merged
merged 5 commits into from
Apr 28, 2021
Merged

Implement flags over native gem & config handling #19

merged 5 commits into from
Apr 28, 2021

Conversation

lukehinds
Copy link
Member

This change allows sigstore subcommands such as --sign and
--verify to layer on top of the existing gem commands. This way
a user can call gem build --sign and sigstore will sign during
the gem build process. Likewise they can run gem install --verify

This also replaces run time flags for config options, namely as
most users are not likely going to use flags. Using config means
that defaults are present and they can be overidden by env vars,
such as export sigstore_fulcio_host=http://127.0.0.1:3000. The
main requirement is that they prefix sigstore_

If flags are still really desired we can rig them back in. I also
removed as config plays easier with overiding subcommands against
the generic gem command.

Signed-off-by: Luke Hinds lhinds@redhat.com

Luke Hinds added 2 commits April 26, 2021 14:31
This change allows sigstore subcommands such as `--sign` and
`--verify` to layer on top of the existing gem commands. This way
a user can call `gem build --sign` and sigstore will sign during
the gem build process. Likewise they can run `gem install --verify`

This also replaces run time flags for config options, namely as
most users are not likely going to use flags. Using config means
that defaults are present and they can be overidden by env vars,
such as `export sigstore_fulcio_host=http://127.0.0.1:3000`. The
main requirement is that they prefix `sigstore_`

If flags are still really desired we can rig them back in. I also
removed as config plays easier with overiding subcommands against
the generic gem command.

Signed-off-by: Luke Hinds <lhinds@redhat.com>
Signed-off-by: Luke Hinds <lhinds@redhat.com>
@lukehinds
Copy link
Member Author

lukehinds commented Apr 26, 2021

Picture is worth a thousands words and all that

image

@lukehinds
Copy link
Member Author

@cpanato just in regards to the license boiler check , do we want license header in config files? There is no code to copyright, its just stuff like host, ports, client ids

@cpanato
Copy link
Member

cpanato commented Apr 26, 2021

@lukehinds make sense Luke, just need to see how to skip those files in the checker

Luke Hinds added 2 commits April 27, 2021 12:30
Signed-off-by: Luke Hinds <lhinds@redhat.com>
Signed-off-by: Luke Hinds <lhinds@redhat.com>
lib/rubygems/sigstore/verify_extend.rb Outdated Show resolved Hide resolved
lib/rubygems/sigstore/config.rb Show resolved Hide resolved
lib/rubygems/sigstore/verify_extend.rb Show resolved Hide resolved
Signed-off-by: Bob Callaway <bob.callaway@gmail.com>
@lukehinds
Copy link
Member Author

Are you ok with this now @bobcallaway , the CI failure is from the boilerplate license check action failing as it expects a license header in a config file.

@cpanato
Copy link
Member

cpanato commented Apr 28, 2021

the tool we are using github.com/google/addlicense does not support a flag to skip files that we don't want to validate.
I will open a PR in the project to add this feature, and if that gets merged we can update the workflow here.

if not get merged, we might want to use the boilerplate check from Kubernetes which are more flexible https://github.com/kubernetes/repo-infra/tree/master/verify

let me know what are your thoughts on this
thanks!

@lukehinds
Copy link
Member Author

the tool we are using github.com/google/addlicense does not support a flag to skip files that we don't want to validate.
I will open a PR in the project to add this feature, and if that gets merged we can update the workflow here.

if not get merged, we might want to use the boilerplate check from Kubernetes which are more flexible https://github.com/kubernetes/repo-infra/tree/master/verify

let me know what are your thoughts on this
thanks!

This is up, but I don't think its an ideal approach as you have to specifically name the files , it would be better to have it ignore extensions:

google/addlicense#73

@cpanato
Copy link
Member

cpanato commented Apr 28, 2021

agree @lukehinds
I did not know that PR, I just quick implemented this: google/addlicense@master...cpanato:skipfiles

@lukehinds
Copy link
Member Author

agree @lukehinds
I did not know that PR, I just quick implemented this: google/addlicense@master...cpanato:skipfiles

I much prefer this approach!

@cpanato
Copy link
Member

cpanato commented Apr 28, 2021

ok, opened a PR: google/addlicense#75 lets see the maintainers thoughts

Copy link
Contributor

@endorama endorama left a comment

Choose a reason for hiding this comment

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

Apart from the mentioned license issue, 👍 from me!

@bobcallaway bobcallaway merged commit 85f6cc7 into sigstore:main Apr 28, 2021
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.

4 participants