Actually call PipelineSpec.Validate(). #1074
Conversation
Call PipelineSpec.Validate() within Pipeline.Validate(), and add tests for Pipeline.Validate(). Previously, none of the PipelineSpec validation logic was actually being called, but the unit tests passed because they manually called PipelineSpec.Validate().
googlebot
added
the
cla: yes
tekton-robot
added
the
size/M
|
/assign @imjasonh |
|
ooooooh boy.. thanks for catching this @EliZucker 😅😅😅🙏 /lgtm |
|
In response to this:
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. |
tekton-robot
added
lgtm
tekton-robot
added
size/L
Refactor tests so that success/failure testing is done within the same function (specified with a bool). Also formatted brackets differently following feeback from @imjasonh.
|
/lgtm Thanks for catching this! 👍 |
EliZucker
force-pushed
the
pipelinespecfix
branch
from
576b680 to
cc7d8aa
Compare
|
In response to this:
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. |
tekton-robot
added
lgtm
|
@imjasonh I'm not sure how to get rid of the "change requested" message blocking merging. Is that resolved from my end or do you have to approve your requested changes? |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bobcatfish, EliZucker, ImJasonH 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:
Approvers can indicate their approval by writing |
That was me, sorry about that. |
No problem, I wasn't super familiar with that GitHub feature so wasn't sure what to do. Also, I did a super minor rewording on the commit message, so another lgtm is needed. Thanks! |
|
/lgtm |
Changes
Call PipelineSpec.Validate() within Pipeline.Validate(), and add tests for Pipeline.Validate().
I was adding logic for array-specific template-string testing, and was confused why my validation wasn't working in my test cluster's pipelines. I eventually realized that PipelineSpec isn't actually being validated, despite all the existing code intended to do so 😆
You can check this yourself by deploying a pipeline with duplicate tasks. There is code and tests specifically written to catch this immediately via the webhook, but it will deploy fine if you actually try it.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe release Task and TaskRun to build and release this image
Release Notes