Refactor Step validation to implement apis.Validatable. by twoGiants · Pull Request #8717 · tektoncd/pipeline

twoGiants · 2025-04-18T09:30:37Z

Changes

As a follow up to #8685 in this small PR the Step validation is refactored to align with the pattern used in pipeline_validation.go, task_validation.go, result_validation.go, etc. meaning implementing the apis.Validatable interface and using a custom type for a list of Step namely StepList.

Now that the Step validation has a public Validate API, the tests can be moved from task_validation_test.go to container_validation_test.go.

Issue #8700.

/kind cleanup

Submitter Checklist

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

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
Has Tests included if any functionality added or changed
pre-commit Passed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

tekton-robot · 2025-04-18T09:41:14Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/task_validation.go	98.6%	97.8%	-0.8

waveywaves · 2025-05-07T10:08:30Z

@twoGiants: PR needs rebase :p

twoGiants · 2025-05-13T18:36:04Z

@twoGiants: PR needs rebase :p

Yes sir! Is in the works and will open a couple more soon 😸

Add `StepList` with a `Validate` method and implement a `Validate` method on `Step`. Move up check for duplicated Step names into `StepList.Validate` to be consistent with the check for duplicated Task names in `pipeline_validation.go`. Issue tektoncd#8700. Signed-off-by: Stanislav Jakuschevskij <stas@two-giants.com>

twoGiants · 2025-05-14T12:49:04Z

/kind cleanup

You can take a look, it's ready @waveywaves. Thank you 😸

Next will be PRs with batches of moved and renamed tests.

waveywaves · 2025-05-14T12:53:24Z

pkg/apis/pipeline/v1/container_validation.go

 	errs = errs.Also(errorIfStepResultReferencedInField(s.Image, "image"))
 	errs = errs.Also(errorIfStepResultReferencedInField(s.Script, "script"))
-	errs = errs.Also(errorIfStepResultReferencedInField(string(s.ImagePullPolicy), "imagePullPoliicy"))
+	errs = errs.Also(errorIfStepResultReferencedInField(string(s.ImagePullPolicy), "imagePullPolicy"))


you're finding a lot of typos hahaha

I have a code spell checker installed 😁 😎

waveywaves · 2025-05-14T12:54:05Z

pkg/apis/pipeline/v1/task_validation.go

 	}

-	errs = errs.Also(validateSteps(ctx, mergedSteps).ViaField("steps"))
+	errs = errs.Also(StepList(mergedSteps).Validate(ctx).ViaField("steps"))


why does it feel like I have already reviewed this 🤔

Probably because its similar to the sidecar Validatable refactoring in #8710. It's basically the same but for the Step :).

Here is a better link to the same line.

ah yess, thank you for sharing

waveywaves · 2025-05-14T12:54:52Z

pkg/apis/pipeline/v1/container_validation.go


-func validateStep(ctx context.Context, s Step, names sets.String) (errs *apis.FieldError) {
+// Validate implements apis.Validatable
+func (s *Step) Validate(ctx context.Context) (errs *apis.FieldError) {


tekton-robot · 2025-05-14T12:57:39Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/task_validation.go	99.1%	99.1%	0.0

vdemeester

/lgtm

tekton-robot · 2025-05-20T11:19:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester, waveywaves

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:

~~pkg/apis/OWNERS~~ [vdemeester,waveywaves]

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

github-project-automation bot added this to Tekton Community Roadmap Apr 18, 2025

tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Apr 18, 2025

github-project-automation bot moved this to Todo in Tekton Community Roadmap Apr 18, 2025

tekton-robot requested review from pritidesai and vdemeester April 18, 2025 09:30

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 18, 2025

twoGiants mentioned this pull request Apr 18, 2025

Refactor step and sidecar validation to implement apis.Validatable. #8700

Closed

25 tasks

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2025

twoGiants force-pushed the issue-8700-refactor-step-validation-to-implement-validatable branch from 70c4547 to b88b80f Compare May 14, 2025 12:45

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 14, 2025

twoGiants changed the title ~~🚧 Refactor Step validation to implement apis.Validatable. 🚧~~ Refactor Step validation to implement apis.Validatable. May 14, 2025

twoGiants marked this pull request as ready for review May 14, 2025 12:46

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 14, 2025

tekton-robot requested a review from jerop May 14, 2025 12:46

tekton-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label May 14, 2025

waveywaves approved these changes May 14, 2025

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 14, 2025

This was referenced May 15, 2025

Move Step Validate and ValidateError tests to container_validation_test.go. #8778

Merged

Move Step step action reference test to container_validation_test.go. #8779

Merged

vdemeester approved these changes May 20, 2025

View reviewed changes

tekton-robot assigned vdemeester May 20, 2025

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

tekton-robot merged commit 1027f72 into tektoncd:main May 20, 2025
20 checks passed

github-project-automation bot moved this from Todo to Done in Tekton Community Roadmap May 20, 2025

Conversation

twoGiants commented Apr 18, 2025 • edited by waveywaves Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Submitter Checklist

Release Notes

Uh oh!

tekton-robot commented Apr 18, 2025

Uh oh!

waveywaves commented May 7, 2025

Uh oh!

twoGiants commented May 13, 2025

Uh oh!

twoGiants commented May 14, 2025

Uh oh!

waveywaves May 14, 2025

Choose a reason for hiding this comment

Uh oh!

twoGiants May 14, 2025

Choose a reason for hiding this comment

Uh oh!

waveywaves May 14, 2025

Choose a reason for hiding this comment

Uh oh!

twoGiants May 14, 2025

Choose a reason for hiding this comment

Uh oh!

twoGiants May 14, 2025

Choose a reason for hiding this comment

Uh oh!

waveywaves May 20, 2025

Choose a reason for hiding this comment

Uh oh!

waveywaves May 14, 2025

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented May 14, 2025

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented May 20, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

twoGiants commented Apr 18, 2025 •

edited by waveywaves

Loading