Move Steps and Sidecars validation to `container_validation.go`. #8685

twoGiants · 2025-04-05T17:36:29Z

Changes

Move step and sidecar validation functions with their call chain to container_validation.go and fix small typos.

Create validateSidecar and move to container_validation.go to be consistent with the validateStep function location. Inline validateSidecarName function into validateSidecar to keep consistency because the fields sc.Image and sc.Script are validated in the caller.

Keep validationSteps and validateSidecars in task_validation.go.

Closes #7442.

/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

twoGiants · 2025-04-05T17:38:41Z

/cc @JeromeJu you can take a look. I believe I caught all the affected functions in the call chain and moved them to container_validation.go. This PR #7538 can probably be closed after mine is merged.

tekton-robot · 2025-04-05T17:46:01Z

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/container_validation.go	100.0%	99.4%	-0.6
pkg/apis/pipeline/v1/task_validation.go	98.6%	98.2%	-0.5

waveywaves · 2025-04-07T09:15:53Z

pkg/apis/pipeline/v1/container_validation.go


+// Validate ensures that a supplied Ref field is populated
+// correctly. No errors are returned for a nil Ref.
+func (ref *Ref) Validate(ctx context.Context) (errs *apis.FieldError) {


do the tests have to be moved as well ?

Great that you bring it up! I checked the tests before opening the PR and wanted to move them but kept them where there are. So here why.

Ref.Validate tests are already in container_valitation_test.go, so nothing to move here.

validateSteps does not have a "direct" test, it's private, it's called in TaskSpec.Validate in task_validation.go:91 and is tested via TaskSpec.Validate here and here and here and in more test functions. That would be a lot of moving and we would either have to change validationSteps to a public function or the container_validation_tests.go to package scoped tests, which are changes I assumed you probably don't want in this PR and not without more thinking.

I proposed in the working group to do a refactoring of validationSteps to the same style you have in Task and Pipeline by implementing apis.Validatable and add a Validate method on Step and create a new type TaskStepList, similar to PipelineTaskList with its own Validate method, in short to align the validateSteps implementation with Task and Pipeline validation. Then some tests can be moved to container_validation_test.go and run against the public Validate method on Step.

My proposal was accepted and @binkkatal was so nice to offer to help and do the review.

Its very similar with validateSidecars just much simpler because there are fewer checks and tests.

Sorry for the long answer. I hope I expressed myself clear. Let me know what you think and if I am on the right track.

I created an issue #8700 for the refactoring and started working on it. Once this PR is merged I will open a couple small PRs for the migration and a bit of cleanup of the tests.

Sorry for the long answer. I hope I expressed myself clear. Let me know what you think and if I am on the right track.

I appreciated it. Thank you.

I created an issue #8700 for the refactoring and started working on it. Once this PR is merged I will open a couple small PRs for the migration and a bit of cleanup of the tests.

noted.

waveywaves · 2025-04-07T09:16:06Z

refactor season 🌴

twoGiants · 2025-04-09T14:00:53Z

refactor season 🌴

It is on 🌴 😎 🌴 .

tekton-robot · 2025-04-09T14:59:59Z

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/container_validation.go	100.0%	99.4%	-0.6
pkg/apis/pipeline/v1/task_validation.go	98.6%	98.2%	-0.5

tekton-robot · 2025-04-12T09:03:30Z

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/container_validation.go	100.0%	99.5%	-0.5
pkg/apis/pipeline/v1/task_validation.go	98.6%	98.0%	-0.6

tekton-robot · 2025-04-12T10:22:48Z

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/container_validation.go	100.0%	99.4%	-0.6
pkg/apis/pipeline/v1/task_validation.go	98.6%	98.2%	-0.4

twoGiants · 2025-04-12T11:04:09Z

Hi @waveywaves, hi @binkkatal! I did some minor improvements to this PR. I describe the changes better in the commit message and in the PR message on top. You can take another look when you find the time. Thank you!

binkkatal · 2025-04-13T14:51:11Z

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/container_validation.go 100.0% 99.4% -0.6
pkg/apis/pipeline/v1/task_validation.go 98.6% 98.2% -0.4

@twoGiants , do we need to worry about the delta in the test cases ?

binkkatal · 2025-04-14T08:35:23Z

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/container_validation.go 100.0% 99.4% -0.6
pkg/apis/pipeline/v1/task_validation.go 98.6% 98.2% -0.4

@twoGiants , do we need to worry about the delta in the test cases ?

The issue #8700 satisfies my question
@twoGiants

twoGiants · 2025-04-14T10:34:02Z

Hi @waveywaves ! Do you have a little time to take a look at the PR? I can break it down in two if its to big. Let me know. Thank you!

pkg/apis/pipeline/v1/container_validation.go

Move step and sidecar validation functions with their call chain to `container_validation.go` and fix small typos. Create `validateSidecar` and move to `container_validation.go` to be consistent with the `validateStep` function location. Inline `validateSidecarName` function into `validateSidecar` to keep consistency because the fields `sc.Image` and `sc.Script` are validated in the caller. Keep `validationSteps` and `validateSidecars` in `task_validation.go`. Issue tektoncd#7442. Signed-off-by: Stanislav Jakuschevskij <[email protected]>

tekton-robot · 2025-04-30T11:04:37Z

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/container_validation.go	100.0%	98.8%	-1.2
pkg/apis/pipeline/v1/task_validation.go	98.3%	98.2%	-0.1

twoGiants · 2025-04-30T12:27:52Z

@waveywaves should be good now. New changes integrated, tests pass.

waveywaves · 2025-05-07T09:52:23Z

pkg/apis/pipeline/v1/container_validation.go


+// Validate ensures that a supplied Ref field is populated
+// correctly. No errors are returned for a nil Ref.
+func (ref *Ref) Validate(ctx context.Context) (errs *apis.FieldError) {


Sorry for the long answer. I hope I expressed myself clear. Let me know what you think and if I am on the right track.

I appreciated it. Thank you.

I created an issue #8700 for the refactoring and started working on it. Once this PR is merged I will open a couple small PRs for the migration and a bit of cleanup of the tests.

noted.

waveywaves · 2025-05-07T09:57:21Z

pkg/apis/pipeline/v1/container_validation.go

 	return nil
 }
+
+func validateStep(ctx context.Context, s Step, names sets.String) (errs *apis.FieldError) {


+1 for colocating step and sidecar related functions in the container validation instead of task validation.

tekton-robot · 2025-05-07T09:57:42Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

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

waveywaves · 2025-05-07T10:00:51Z

@twoGiants this looks good to me, going forward and hitting that lgtm considering that similar code is being colocated and this doesn't introduce or break any existing functionality and recorganizes existing code a little better

/lgtm

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

tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Apr 5, 2025

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

tekton-robot requested review from afrittoli and pritidesai April 5, 2025 17:36

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

twoGiants changed the title ~~cleanup: move step and sidecar validation~~ Cleanup: refactor Steps and Sidecars to container_validation. Apr 5, 2025

tekton-robot requested a review from JeromeJu April 5, 2025 17:38

twoGiants marked this pull request as ready for review April 5, 2025 17:42

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 Apr 5, 2025

tekton-robot requested review from abayer and vdemeester April 5, 2025 17:42

twoGiants changed the title ~~Cleanup: refactor Steps and Sidecars to container_validation.~~ Cleanup: move Steps and Sidecars validation to container_validation.go. Apr 5, 2025

twoGiants changed the title ~~Cleanup: move Steps and Sidecars validation to container_validation.go.~~ Move Steps and Sidecars validation to container_validation.go. Apr 5, 2025

waveywaves reviewed Apr 7, 2025

View reviewed changes

binkkatal approved these changes Apr 9, 2025

View reviewed changes

twoGiants force-pushed the issue-7442-move-step-and-sidecar-validation branch from d8104b6 to 2f83d73 Compare April 9, 2025 14:49

twoGiants force-pushed the issue-7442-move-step-and-sidecar-validation branch from 2f83d73 to 71aaff7 Compare April 12, 2025 08:53

twoGiants force-pushed the issue-7442-move-step-and-sidecar-validation branch from 71aaff7 to a7e984e Compare April 12, 2025 10:13

twoGiants mentioned this pull request Apr 13, 2025

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

Closed

25 tasks

This was referenced Apr 15, 2025

Refactor sidecar validation to implement apis.Validatable. #8710

Merged

Raise test coverage in task_validation.go and container_validation.go. #8714

Merged

Refactor Step validation to implement apis.Validatable. #8717

Merged

twoGiants commented Apr 26, 2025

View reviewed changes

pkg/apis/pipeline/v1/container_validation.go Outdated Show resolved Hide resolved

twoGiants mentioned this pull request Apr 26, 2025

feat: promote StepActions to GA #8546

Merged

8 tasks

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

twoGiants force-pushed the issue-7442-move-step-and-sidecar-validation branch from a7e984e to 1624848 Compare April 30, 2025 10:52

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

waveywaves approved these changes May 7, 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 7, 2025

tekton-robot assigned waveywaves May 7, 2025

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

tekton-robot merged commit 58cd6e2 into tektoncd:main May 7, 2025
20 checks passed

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

twoGiants mentioned this pull request May 13, 2025

fix:Refactored Steps and Sidecars to container_validation #7454 #7538

Closed

7 tasks

Move Steps and Sidecars validation to container_validation.go. #8685

Move Steps and Sidecars validation to container_validation.go. #8685

Uh oh!

Conversation

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

Uh oh!

Changes

Submitter Checklist

Release Notes

Uh oh!

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

Uh oh!

Uh oh!

tekton-robot commented Apr 5, 2025

Uh oh!

waveywaves Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

twoGiants Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

waveywaves May 7, 2025

Choose a reason for hiding this comment

Uh oh!

waveywaves commented Apr 7, 2025

Uh oh!

twoGiants commented Apr 9, 2025

Uh oh!

tekton-robot commented Apr 9, 2025

Uh oh!

tekton-robot commented Apr 12, 2025

Uh oh!

tekton-robot commented Apr 12, 2025

Uh oh!

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

Uh oh!

Uh oh!

binkkatal commented Apr 13, 2025

Uh oh!

binkkatal commented Apr 14, 2025

Uh oh!

twoGiants commented Apr 14, 2025

Uh oh!

Uh oh!

tekton-robot commented Apr 30, 2025

Uh oh!

twoGiants commented Apr 30, 2025

Uh oh!

waveywaves May 7, 2025

Choose a reason for hiding this comment

Uh oh!

waveywaves May 7, 2025

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented May 7, 2025

Uh oh!

waveywaves commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Move Steps and Sidecars validation to `container_validation.go`. #8685

Move Steps and Sidecars validation to `container_validation.go`. #8685

twoGiants commented Apr 5, 2025 •

edited

Loading

twoGiants commented Apr 5, 2025 •

edited

Loading

twoGiants Apr 9, 2025 •

edited

Loading

twoGiants commented Apr 12, 2025 •

edited

Loading

waveywaves commented May 7, 2025 •

edited

Loading