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

Refactor fulcio signer to take in KeyOpts. #1788

Merged
merged 1 commit into from Apr 22, 2022

Conversation

wlynch
Copy link
Member

@wlynch wlynch commented Apr 22, 2022

Summary

This commit should not have change in behavior. It changes the fulcio signer
to take in the KeyOpts struct instead of passing individual parameters through,
as well as moves common code dependent on the values to the same place.

The motivation behind this change is to make it easier to add new options without
needing to plumb through another param to the already long list.

To avoid circular dependencies (sign -> fulcio -> sign), KeyOpts was moved to the
options package.

Ticket Link

Part of #1785 (wanted to make this change first to make adding the option easier)

Release Note

NONE

@wlynch wlynch force-pushed the fulcio-refactor branch 2 times, most recently from 8fc5bcf to b37d1f2 Compare April 22, 2022 16:19
@wlynch wlynch mentioned this pull request Apr 22, 2022
This commit should not have change in behavior. It changes the fulcio signer
to take in the KeyOpts struct instead of passing individual parameters through,
as well as moves common code dependent on the values to the same place.

The motivation behind this change is to make it easier to add new options without
needing to plumb through another param to the already long list.

To avoid circular dependencies (sign -> fulcio -> sign), KeyOpts was moved to the
options package.

Signed-off-by: Billy Lynch <billy@chainguard.dev>
@codecov-commenter
Copy link

Codecov Report

Merging #1788 (0d0e5b6) into main (afaf0a3) will decrease coverage by 0.01%.
The diff coverage is 3.44%.

@@            Coverage Diff             @@
##             main    #1788      +/-   ##
==========================================
- Coverage   31.46%   31.45%   -0.02%     
==========================================
  Files         144      144              
  Lines        8893     8896       +3     
==========================================
  Hits         2798     2798              
- Misses       5758     5761       +3     
  Partials      337      337              
Impacted Files Coverage Δ
cmd/cosign/cli/attest.go 0.00% <0.00%> (ø)
cmd/cosign/cli/policy_init.go 1.40% <0.00%> (ø)
cmd/cosign/cli/sign.go 0.00% <0.00%> (ø)
cmd/cosign/cli/sign/sign.go 14.69% <0.00%> (+0.33%) ⬆️
cmd/cosign/cli/sign/sign_blob.go 0.00% <0.00%> (ø)
cmd/cosign/cli/signblob.go 0.00% <0.00%> (ø)
cmd/cosign/cli/verify.go 0.00% <0.00%> (ø)
cmd/cosign/cli/verify/verify_blob.go 10.42% <0.00%> (ø)
cmd/cosign/cli/fulcio/fulcio.go 22.10% <7.14%> (-2.90%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update afaf0a3...0d0e5b6. Read the comment docs.

@dlorenc dlorenc merged commit 8368bad into sigstore:main Apr 22, 2022
@github-actions github-actions bot added this to the v1.8.0 milestone Apr 22, 2022
wlynch added a commit to wlynch/cosign that referenced this pull request Apr 25, 2022
wlynch added a commit to wlynch/cosign that referenced this pull request Apr 25, 2022
This reverts commit 8368bad.

Signed-off-by: Billy Lynch <billy@chainguard.dev>
dlorenc pushed a commit that referenced this pull request Apr 25, 2022
This reverts commit 8368bad.

Signed-off-by: Billy Lynch <billy@chainguard.dev>
mlieberman85 pushed a commit to mlieberman85/cosign that referenced this pull request May 6, 2022
This commit should not have change in behavior. It changes the fulcio signer
to take in the KeyOpts struct instead of passing individual parameters through,
as well as moves common code dependent on the values to the same place.

The motivation behind this change is to make it easier to add new options without
needing to plumb through another param to the already long list.

To avoid circular dependencies (sign -> fulcio -> sign), KeyOpts was moved to the
options package.

Signed-off-by: Billy Lynch <billy@chainguard.dev>
mlieberman85 pushed a commit to mlieberman85/cosign that referenced this pull request May 6, 2022
…igstore#1798)

This reverts commit 8368bad.

Signed-off-by: Billy Lynch <billy@chainguard.dev>
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.

None yet

3 participants