cliplugin: convert module to package only#1956
Conversation
|
Lint errors should be fixed after merging #1958. Afterwards, we can remove that PR's explicit commands to run golangci-lint the the cliplugin directory. |
|
@haydentherapper @bobcallaway |
|
note: the stuck CI is expected, we'll remove that as a requirement |
|
@haydentherapper It may be because the job's changed names, because of the matrix? https://github.com/orgs/community/discussions/26698#discussioncomment-3252954 |
6891509 to
0a7e2ab
Compare
|
I re-did the commits to follow a better flow. |
| github.com/davecgh/go-spew v1.1.1 | ||
| github.com/sigstore/sigstore v1.8.10 | ||
| github.com/sigstore/sigstore/pkg/signature/kms/cliplugin v0.0.0-00010101000000-000000000000 | ||
| github.com/sigstore/sigstore v0.0.0-00010101000000-000000000000 |
There was a problem hiding this comment.
this looks strange, perhaps it doesn't matter with the replace on line 5?
There was a problem hiding this comment.
That's right. Pinning an actual version won't be useful, I think.
Signed-off-by: Ramon Petgrave <32398091+ramonpetgrave64@users.noreply.github.com> Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Ramon Petgrave <32398091+ramonpetgrave64@users.noreply.github.com> Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Ramon Petgrave <32398091+ramonpetgrave64@users.noreply.github.com> Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Ramon Petgrave <32398091+ramonpetgrave64@users.noreply.github.com> Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Ramon Petgrave <32398091+ramonpetgrave64@users.noreply.github.com> Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Ramon Petgrave <32398091+ramonpetgrave64@users.noreply.github.com> Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
741b040 to
652ef9e
Compare
pkg/signature/kms/kms.go
Outdated
| return sv, nil | ||
| } | ||
| return nil, &ProviderNotFoundError{ref: keyResourceID} | ||
| // We don't return a ProviderNotFoundError because cosign currently interprets those in a confusing way: |
There was a problem hiding this comment.
This library is used outside of Cosign so we shouldn't be making changes just because of how Cosign outputs this error.
Isn't it more accurate to log that it is an unrecognized scheme rather than a missing executable? Whether the KMS provider is an executable or module is a detail the user doesn't need to be aware of.
I think we can get the best of both worlds - wrap the original error from cliplugin.LoadSignerVerifier in a ProviderNotFoundError.
There was a problem hiding this comment.
changed, and added tests
|
|
||
| // SignerVerifier creates and verifies digital signatures over a message using a KMS service | ||
| // It is a wrapper for signerverifier.SignerVerifier. | ||
| type SignerVerifier interface { |
There was a problem hiding this comment.
Rather than modify this interface, can we leave this as-is and just redefine the interface in an internal package under pkg/signature/kms/cliplugin, like pkg/signature/kms/cliplugin/internal/signerverifier?
There was a problem hiding this comment.
That works! though we have to copy the contents of the interface. It's okay, because if the two interfaces are ever out of sync, I see we would get a compiler error since cliplugin.LoadsignerVerifier()'s return value would not satisfy the return value of Get().
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Summary
Pending #1948, #1958
branch diff:
Moves the details of
kms.signerVerifierto a new bridge packagesignerverifier.SignerVerifierto avoid import cycles.Marks
ProviderNotFoundErrorfor deprecation.Converts cliplugin to be a package only, not a module. This lets consumers, such as cosign, avoid having to load the module like below, and its okay because there are no new dependencies.
To start using cliplugin, consumers need only bump the version of sigstore.
Testing
Example error
Release Note
cliplugin is now a package only, not a module that needs to be loaded for initialization. There is no need to explicitly import cliplugin.
Documentation
No doc changes.