Fix default params in attestation invocation by lcarva · Pull Request #449 · tektoncd/chains

lcarva · 2022-05-16T23:40:01Z

Default parameters should always be overwritten by values from the
taskrun spec. This is how Tekton Pipelines behaves. The attestation
should also follow the same rule.

This also fixes zero-value default value representation, i.e. an empty
string or an empty array.

The representation of parameter values is now also consistent. Prior to
this commit, %v on the Param was used for default values while
.StringVal or .ArrayVal was used for non-default values.

Signed-off-by: Luiz Carvalho lucarval@redhat.com

tekton-robot · 2022-05-16T23:40:16Z

Hi @lcarva. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

priyawadhwa · 2022-05-17T02:52:38Z

/ok-to-test

tekton-robot · 2022-05-17T02:54:57Z

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/formats/intotoite6/intotoite6.go	86.4%	90.4%	4.1

lcarva · 2022-05-17T13:36:29Z

 			if p.Default != nil {
-				v := p.Default.StringVal
-				if v == "" {
+				// TODO: Consider using p.Default.MarshalJSON()


@priyawadhwa, should I just do this now?

Ok, I added a commit to do this. Let me know what you think. I can easily drop it if necessary.

@priyawadhwa, please take a look when you have a chance 🙏

tekton-robot · 2022-05-17T14:44:44Z

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/formats/intotoite6/intotoite6.go	86.4%	87.7%	1.4

tekton-robot · 2022-05-17T14:46:46Z

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/formats/intotoite6/intotoite6.go	86.4%	87.0%	0.6

lcarva · 2022-05-17T15:32:43Z

/retest

lcarva · 2022-05-31T17:55:21Z

@priyawadhwa, @wlynch ptal 🙏

wlynch

Overall looks good! Few small things.

wlynch · 2022-06-06T16:24:27Z

@@ -66,10 +66,10 @@ func TestCreatePayload1(t *testing.T) {
 			},
 			Invocation: slsa.ProvenanceInvocation{
 				Parameters: map[string]string{


I wonder if we should use map[string]ArrayOrString (https://pkg.go.dev/github.com/tektoncd/pipeline@v0.36.0/pkg/apis/pipeline/v1beta1#ArrayOrString) here instead and not bother with the string encoding. 🤔 wdyt?

Yeah, that's a better approach. I'll give that a shot.

Done. Here's an example from a dummy task I used:

🐚 cosign download attestation quay.io/lucarval/minimal-container:latest | jq '.payload | @base64d | fromjson | .predicate.invocation.parameters' { "BUILDER_IMAGE": "quay.io/buildah/stable:v1.18.0", "BUILD_EXTRA_ARGS": "", "CONTEXT": ".", "DOCKERFILE": "./Dockerfile", "FORMAT": "oci", "IMAGE": "quay.io/lucarval/minimal-container:latest", "PUSH_EXTRA_ARGS": "", "SKIP_PUSH": "false", "STORAGE_DRIVER": "vfs", "SUPER_ARRAY": [ "hello", "array" ], "TLSVERIFY": "true" }

Default parameters should always be overwritten by values from the taskrun spec. This is how Tekton Pipelines behaves. The attestation should also follow the same rule. This also fixes zero-value default value representation, i.e. an empty string or an empty array. The representation of parameter values is now also consistent. Prior to this commit, %v on the Param was used for default values while .StringVal or .ArrayVal was used for non-default values. Signed-off-by: Luiz Carvalho <lucarval@redhat.com>

Tekton Pipeline allows two different types of parameters, string and array of strings. Represent the correct data type in the attestation. Signed-off-by: Luiz Carvalho <lucarval@redhat.com>

tekton-robot · 2022-06-06T18:35:31Z

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/formats/intotoite6/intotoite6.go	86.4%	89.7%	3.4

wlynch · 2022-06-23T16:25:31Z

/lgtm

lcarva · 2022-06-23T16:31:10Z

@priyawadhwa, need an approval now 🙏

priyawadhwa

LGTM

tekton-robot · 2022-06-23T16:33:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: priyawadhwa

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [priyawadhwa]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tekton-robot requested review from mattmoor and mpeters May 16, 2022 23:40

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 16, 2022

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 17, 2022

lcarva commented May 17, 2022

View reviewed changes

lcarva force-pushed the fix-invocations-params branch from bfb6482 to c8a057c Compare May 17, 2022 14:44

wlynch reviewed Jun 6, 2022

View reviewed changes

lcarva added 2 commits June 6, 2022 14:05

Use ArrayOrString invocation params

2dafdfc

Tekton Pipeline allows two different types of parameters, string and array of strings. Represent the correct data type in the attestation. Signed-off-by: Luiz Carvalho <lucarval@redhat.com>

lcarva force-pushed the fix-invocations-params branch from c8a057c to 2dafdfc Compare June 6, 2022 18:34

tekton-robot assigned wlynch Jun 23, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 23, 2022

priyawadhwa approved these changes Jun 23, 2022

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 23, 2022

tekton-robot merged commit bf1b223 into tektoncd:main Jun 23, 2022

lcarva mentioned this pull request Aug 17, 2022

TEP-84: PipelineRun Attestations #436

Merged

Uh oh!

Conversation

lcarva commented May 16, 2022

Uh oh!

tekton-robot commented May 16, 2022

Uh oh!

priyawadhwa commented May 17, 2022

Uh oh!

tekton-robot commented May 17, 2022

Uh oh!

lcarva May 17, 2022

Choose a reason for hiding this comment

Uh oh!

lcarva May 17, 2022

Choose a reason for hiding this comment

Uh oh!

lcarva May 23, 2022

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented May 17, 2022

Uh oh!

tekton-robot commented May 17, 2022

Uh oh!

lcarva commented May 17, 2022

Uh oh!

lcarva commented May 31, 2022

Uh oh!

wlynch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wlynch Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

lcarva Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

lcarva Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented Jun 6, 2022

Uh oh!

wlynch commented Jun 23, 2022

Uh oh!

lcarva commented Jun 23, 2022

Uh oh!

priyawadhwa left a comment

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented Jun 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants