Move external paramenters and resolved dependencies logic out of v2alpha3 #1109

renzodavid9 · 2024-04-29T13:25:40Z

Related with: #1065

This is a refactor in preparation for v2alpha4 which uses in its majority the same logic as v2alpha3.

Logic and tests moved to chains/formats/slsa/internal to be able to use it from different versions:

chains/formats/slsa/v2alpha3/internal/external_parameters/external_parameters.go => chains/formats/slsa/internal/external_parameters/external_parameters.go
chains/formats/slsa/v2alpha3/internal/resolved_dependencies/resolved_dependencies.go => chains/formats/slsa/internal/resolved_dependencies/resolved_dependencies.go

Current behavior shouldn't change with these changes.

This refactor will be use as base to enable v2alpha4 for TaskRuns, diff can be preview here

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs included if any changes are user facing
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

tekton-robot · 2024-04-29T13:25:50Z

Hi @renzodavid9. 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.

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.

chitrangpatel · 2024-04-29T13:32:30Z

/ok-to-test

tekton-robot · 2024-04-29T13:36:56Z

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/slsa/internal/external_parameters/external_parameters.go	100.0%	87.5%	-12.5
pkg/chains/formats/slsa/internal/resolved_dependencies/resolved_dependencies.go	60.0%	87.5%	27.5

pkg/chains/formats/slsa/internal/external_parameters/external_parameters.go

tekton-robot · 2024-04-29T21:25:50Z

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/slsa/internal/resolved_dependencies/resolved_dependencies.go	60.0%	87.5%	27.5

lcarva

Nice! Just left a few minor comments.

lcarva · 2024-05-01T17:15:50Z

pkg/chains/formats/slsa/internal/resolved_dependencies/resolved_dependencies.go

+			rd, err := addTasks(tr)
+			if err != nil {
+				logger.Errorf("error storing taskRun %s, error: %s", t.Name, err)
+				continue


Should the error be returned instead?

From what I can understand, looks like the idea was to be able to process the other tasks even if one of them couldn't be processed 🤔, returning here will change this behavior; this is the current behavior for v2alpha3. Also from what I can see, the only way to get an error here is from the AddTektonTaskDescriptor function, from the json.Marshal call.

pkg/chains/formats/slsa/internal/resolved_dependencies/resolved_dependencies.go

lcarva · 2024-05-01T17:26:21Z

pkg/chains/formats/slsa/internal/resolved_dependencies/resolved_dependencies.go

+	// add task resources
+	// =====
+	// convert to v1beta1 and add any task resources
+	serializedResources := tro.Annotations["tekton.dev/v1beta1-spec-resources"]


Can we move the annotation name to a variable?

Do we need to only perform the steps below if serializedResources is not empty?

Sure! annotation moved to a variable.

If serializedResources is empty the json.Marshall call will fail, and won't set the shouldReplace to true. We still have to try to get the results from v1beta1 TaskRun object, but we are reading the taskrun resources from another source. So I think it make sense to not check if serializedResources is empty.

pkg/chains/formats/slsa/internal/resolved_dependencies/resolved_dependencies_test.go

tekton-robot · 2024-05-01T17:33:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chitrangpatel, 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:

~~OWNERS~~ [chitrangpatel,lcarva]

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

…pha3 This is a refactor in preparation for v2alpha4 which uses in its majority the same logic as v2alpha3. Logici and tests moved to chains/formats/slsa/internal to be able to use it from different versions. Current behavior shouldn't change with these changes.

tekton-robot · 2024-05-02T16:13:56Z

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/slsa/internal/resolved_dependencies/resolved_dependencies.go	60.0%	87.6%	27.6

chitrangpatel · 2024-05-02T16:18:09Z

Thanks @renzodavid9! If there are still nits that need to be resolved, we can do that in the next PR.
Let's unblock your work.
/lgtm

chitrangpatel · 2024-05-02T16:33:38Z

/retest

tekton-robot requested review from chitrangpatel and lcarva April 29, 2024 13:25

tekton-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 29, 2024

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 Apr 29, 2024

chitrangpatel reviewed Apr 29, 2024

View reviewed changes

pkg/chains/formats/slsa/internal/external_parameters/external_parameters.go Show resolved Hide resolved

chitrangpatel reviewed Apr 29, 2024

View reviewed changes

pkg/chains/formats/slsa/internal/external_parameters/external_parameters.go Show resolved Hide resolved

chitrangpatel approved these changes Apr 29, 2024

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 29, 2024

renzodavid9 force-pushed the 1065-stepactions-refactor-for-v2alpha4 branch from c1c25c3 to a8bcb7c Compare April 29, 2024 21:19

lcarva approved these changes May 1, 2024

View reviewed changes

renzodavid9 force-pushed the 1065-stepactions-refactor-for-v2alpha4 branch from a8bcb7c to 2e2a4b1 Compare May 2, 2024 16:09

tekton-robot assigned chitrangpatel May 2, 2024

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 2, 2024

tekton-robot merged commit 5986f44 into tektoncd:main May 2, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move external paramenters and resolved dependencies logic out of v2alpha3 #1109

Move external paramenters and resolved dependencies logic out of v2alpha3 #1109

renzodavid9 commented Apr 29, 2024 •

edited by chitrangpatel

Loading

tekton-robot commented Apr 29, 2024

chitrangpatel commented Apr 29, 2024

tekton-robot commented Apr 29, 2024

tekton-robot commented Apr 29, 2024

lcarva left a comment

lcarva May 1, 2024

renzodavid9 May 2, 2024

lcarva May 1, 2024

renzodavid9 May 2, 2024

tekton-robot commented May 1, 2024

tekton-robot commented May 2, 2024

chitrangpatel commented May 2, 2024

chitrangpatel commented May 2, 2024

Move external paramenters and resolved dependencies logic out of v2alpha3 #1109

Move external paramenters and resolved dependencies logic out of v2alpha3 #1109

Conversation

renzodavid9 commented Apr 29, 2024 • edited by chitrangpatel Loading

Submitter Checklist

tekton-robot commented Apr 29, 2024

chitrangpatel commented Apr 29, 2024

tekton-robot commented Apr 29, 2024

tekton-robot commented Apr 29, 2024

lcarva left a comment

Choose a reason for hiding this comment

lcarva May 1, 2024

Choose a reason for hiding this comment

renzodavid9 May 2, 2024

Choose a reason for hiding this comment

lcarva May 1, 2024

Choose a reason for hiding this comment

renzodavid9 May 2, 2024

Choose a reason for hiding this comment

tekton-robot commented May 1, 2024

tekton-robot commented May 2, 2024

chitrangpatel commented May 2, 2024

chitrangpatel commented May 2, 2024

renzodavid9 commented Apr 29, 2024 •

edited by chitrangpatel

Loading