Add spdxjson and cyclonedx as supported predicate types #25
Conversation
Signed-off-by: Josh Dolitsky <josh@dolit.ski>
| @@ -165,7 +165,7 @@ func (a *Attestation) Validate(ctx context.Context) *apis.FieldError { | |||
| } | |||
| if a.PredicateType == "" { | |||
| errs = errs.Also(apis.ErrMissingField("predicateType")) | |||
| } else if a.PredicateType != "custom" && a.PredicateType != "slsaprovenance" && a.PredicateType != "spdx" && a.PredicateType != "link" && a.PredicateType != "vuln" { | |||
| } else if a.PredicateType != "custom" && a.PredicateType != "slsaprovenance" && a.PredicateType != "spdx" && a.PredicateType != "spdxjson" && a.PredicateType != "cyclonedx" && a.PredicateType != "link" && a.PredicateType != "vuln" { | |||
There was a problem hiding this comment.
This is begging to be a validPredicates.Contains(a.PredicateType) using something like https://pkg.go.dev/k8s.io/apimachinery/pkg/util/sets, or just a map[string]struct{}{}.
There was a problem hiding this comment.
good call. it hath been done
Signed-off-by: Josh Dolitsky <josh@dolit.ski>
| @@ -165,7 +169,7 @@ func (a *Attestation) Validate(ctx context.Context) *apis.FieldError { | |||
| } | |||
| if a.PredicateType == "" { | |||
| errs = errs.Also(apis.ErrMissingField("predicateType")) | |||
| } else if a.PredicateType != "custom" && a.PredicateType != "slsaprovenance" && a.PredicateType != "spdx" && a.PredicateType != "link" && a.PredicateType != "vuln" { | |||
| } else if !validPredicateTypes.Has(a.PredicateType) { | |||
| // TODO(vaikas): The above should be using something like: | |||
There was a problem hiding this comment.
lol I didn't even see this comment before. Is the import loop still a problem? It'd be nice not to have to maintain this list in two places.
There was a problem hiding this comment.
Sounds like that's https://github.com/sigstore/cosign/blob/main/cmd/cosign/cli/options/predicate.go#L40
The good news is, I don't think that would cause a dependency cycle.
The bad news is, it's a new dependency on cmd/cosign 😭
I think we could probably just maintain two sets for now, and maybe move both into sigstore/sigstore (or into cosign/pkg/something). Doesn't need to block this PR though I think.
There was a problem hiding this comment.
Yeah, +1 for just having a local copy and move it somewhere else. But, yes, agree on not being part of this PR. We should remove this TODO from here though, since its' addressed by the TODO above. But that can be done in a follow on as well, I'll do it 👍
Please see the following PRs in cosign where these new types are added: