v1: Rewrite OCI storage to use new interface #842
Conversation
tekton-robot
added
the
size/XL
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
| ## Signables | ||
|
|
||
| At it's core, Chains is basically an ETL pipeline. We Extract artifacts from run | ||
| objects, Transform and sign them, then Load them into storage. |
There was a problem hiding this comment.
This is a good explanation. It would be great to show how they map to the Signable, Payloader, Signer and Storer interfaces below.
I think you mean something like this: We Extract artifacts from run objects (Signable), Transform (Payloader) and sign (Signer) them, then Load them into storage (Storer). If so, that makes perfect sense to me.
Maybe in the future, we may want to rename these interfaces. Signer and Storer are clear. Signable is confusing because it has nothing to do with signing - maybe Extractor? Payloader is a bit ambiguous - Transformer then Transformer.Transform? I'm nitpicking. Just trying to make this easier for newcomers to understand.
There was a problem hiding this comment.
Yeah I agree. I'm somewhat biasing on existing interface names at the moment. Happy to rename these (though probably in a separate PR).
tekton-robot
added
the
needs-rebase
tekton-robot
removed
the
needs-rebase
|
The following is the coverage report on the affected files.
|
| var repo *name.Repository | ||
| if r := cfg.Storage.OCI.Repository; r != "" { | ||
| var opts []name.Option | ||
| if cfg.Storage.OCI.Insecure { |
There was a problem hiding this comment.
I believe this should be moved outside of the outer if-block. At least today, this config option applies to any repo, not just Storage.OCI.Repository.
There was a problem hiding this comment.
It's a bit confusing, but this was already happening outside of this block - this was only handling the "you asked us to store the objects in another repo" resolution.
I agree this isn't super clear though, so refactored this a bit more to try and consolidate this (and as an added bonus, removed the need to pass config.Config in!)
|
The following is the coverage report on the affected files.
|
Removes the need to take in a config object.
|
The following is the coverage report on the affected files.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lcarva The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
|
/lgtm |
|
/lgtm |
|
The following is the coverage report on the affected files.
|
Changes
This change is part of a larger effort for refactoring external facing libraries to make them easier to support for v1+: #780
This introduces a new signing interface using generics. This allows us to cut out much of the type switching and parameter plumbing by making the key fields that storers need to make decisions on how to store signatures / attestations:
This change marks the existing Backend implementation as deprecated, but is intended to be backwards compatible and still supported for the time being. There should be no change in user behavior.
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
Release Notes