Refactor step and sidecar validation to implement `apis.Validatable`.

[twoGiants]

Note

This issue is done. It will be merged in small batches once #8685 is merged.

Summary

While working on issue #7442 in #8685 it stood out that step and sidecar validation is implemented inconsistent with the validation of the other core structs like Pipeline, PipelineSpec, Task, TaskSpec, etc.. They implement the apis.Validatable interface and are tested against the Validate method.

After merging of #8685 step and sidecar validation will be in container_validation.go but their tests in task_validation_test.go. They can not be moved without changing the scope of container_validation_test.go or making the step and sidecar validation functions public.

This refactoring should resolve the mentioned inconsistencies.

Tasks

Main branch with all the changes below commit by commit.

Steps

• PR Refactor Step validation to implement apis.Validatable. #8717
  • Add a StepList type with a Validate method. It should replace validateSteps function.
  • Implement a Validate method on Step which should replace validateStep function.
  • Move names validation up to the caller.
  • Move tests for new Validate methods to container_validation_test.go. The old tests can be refactored to run against the new Validate methods. Affected tests are:
    • Commit with a first batch of moved tests:
      • TestTaskSpecValidateErrorWithArtifactsRefFlagNotEnabled
      • TestTaskSpecValidateSuccessWithArtifactsRefFlagEnabled
      • Open PR, once the others are merged:
        • Move Step artifacts flag tests to container_validation_test.go. #8769
    • Comimt with a second batch of moved tests:
      • TestTaskSpecValidateErrorWithStepActionRef_CreateUpdateEvent
      • TestTaskSpecValidateErrorWithStepActionRef
      • Open PR, once the others are merged:
        • Move Step ref tests to container_validation_test.go. #8772
    • Commit with a third batch of moved tests:
      • TestStepOnError
      • TestIncompatibleAPIVersions
      • Open PR, once the others are merged:
        • Move Step onError and API version tests to container_validation_test.go. #8773
    • Commit with a fourth batch of moved tests:
      • TestTaskSpecValidateErrorWithStepResultRef
      • TestTaskSpecValidateErrorWithArtifactsRef
      • Open PR, once the others are merged:
        • Move Step ResultRef and ArtifactsRef tests to container_validation_test.go. #8777
    • Commit with a fifth batch of moved tests:
      • TestTaskSpecValidateError
      • TestTaskSpecValidate
      • Open PR, once the others are merged:
        • Move Step Validate and ValidateError tests to container_validation_test.go. #8778
    • Commit with a last moved test:
      • TestTaskSpecStepActionReferenceValidate
      • Open PR, once the others are merged:
        • Move Step step action reference test to container_validation_test.go. #8779

Sidecar

• PR Refactor sidecar validation to implement apis.Validatable. #8710
  • Do a similar refactoring for validateSidecars which should be simpler. Affected tests are:
    • TestTaskSpecValidateErrorSidecarName
    • TestTaskSpecValidateErrorSidecars

Test Coverage

• PR Raise test coverage in task_validation.go and container_validation.go. #8714
  • Improve coverage by adding test cases for conditions which are not covered. Check non-covered conditions:

go test ./pkg/apis/pipeline/v1 -coverprofile=coverage.out
go tool cover -html=coverage.out

/kind cleanup

/good-first-issue

/assign

[tekton-robot]

@twoGiants:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

Summary

While working on issue #7442 in #8685 it stood out that step and sidecar validation is implemented inconsistent with the validation of the other core structs like Pipeline, PipelineSpec, Task, TaskSpec, etc.. They implement the apis.Validatable interface and are tested against the Validate method.

After merging of #8685 step and sidecar validation will be in container_validation.go but their tests in task_validation_test.go. They can not be moved without changing the scope of container_validation_test.go or making the step and sidecar validation functions public.

This refactoring should resolve the mentioned inconsistencies.

Add a TaskStepList type with a Validate method. It should replace validateSteps function. Implement a Validate method on Step which should replace validateStep function. Move names validation up to the caller. Once this is done the tests can be moved to container_validation_test.go and refactored to run against the new Validate methods.

Do a similar refactoring for validateSidecars which should be simpler.

/kind cleanup

/good-first-issue

/assign

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.

[twoGiants]

Hi @binkkatal! I will put you as a reviewer on this issue. I think I will manage this one, but you can support if you like and have time. Thank you 😸

[binkkatal]

@twoGiants , i can pick this up
/assign