fix ValidateStepResultsVariables to validate stepResults only #8264
Conversation
tekton-robot
added
release-note-none
jkhelil
force-pushed
the
fix_validateTaskResultsVariables
branch
from
a7cd2bb to
8e0fa48
Compare
|
/kind bug |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
jkhelil
force-pushed
the
fix_validateTaskResultsVariables
branch
from
8e0fa48 to
0c6cd04
Compare
tekton-robot
added
size/XS
jkhelil
force-pushed
the
fix_validateTaskResultsVariables
branch
from
0c6cd04 to
18dee1a
Compare
jkhelil
changed the title
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
jkhelil
force-pushed
the
fix_validateTaskResultsVariables
branch
from
18dee1a to
7c5e691
Compare
tekton-robot
added
size/S
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
jkhelil
force-pushed
the
fix_validateTaskResultsVariables
branch
2 times, most recently
from
e078cdd to
98e4c6f
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
jkhelil
force-pushed
the
fix_validateTaskResultsVariables
branch
from
98e4c6f to
f1fc5c5
Compare
|
The following is the coverage report on the affected files.
|
tekton-robot
added
the
approved
jkhelil
force-pushed
the
fix_validateTaskResultsVariables
branch
from
f1fc5c5 to
5a87c4a
Compare
tekton-robot
added
size/M
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester 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 |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
| }, { | ||
| name: "step script refers to nonexistent result", | ||
| fields: fields{ | ||
| Image: "my-image", | ||
| Script: ` | ||
| #!/usr/bin/env bash | ||
| date | tee $(results.non-exist.path)`, | ||
| Results: []v1.StepResult{{Name: "a-result"}}, | ||
| }, | ||
| expectedError: apis.FieldError{ | ||
| Message: `non-existent variable in "\n\t\t\t#!/usr/bin/env bash\n\t\t\tdate | tee $(results.non-exist.path)"`, | ||
| Paths: []string{"script"}, | ||
| }, |
There was a problem hiding this comment.
The fact that you have to remove this test here highlights another problem in the current codebase.
You want to fix an issue in how we validate an inline step in a Task definition, and this has a side effect on the validation of a StepAction, which should not happen.
In the task-stepaction-results.yaml example that you provided, an inline step references to a Task result, which is valid and should work, and doesn't work today, which is the problem that needs to be fixed.
This stems from the fact that ValidateStepResultsVariables does not have enough context to validate Task level references, which are validated instead by ValidateTaskResultsVariables as you mentioned.
The change you proposed, however, modifies code used for validation of StepActions, which is not ok, as exemplified by the test you removed, which should continue to pass. The author of a StepAction has no knowledge about the Tasks that may use it, and should not reference any variable outside of the known context.
A correct fix for the issue should include, on top of the fix already done, a change in the stepaction_validation code, which, instead of using ValidateStepResultsVariables from the task validation, should define its own ValidateStepActionResultsVariables, which should look like what ValidateStepResultsVariables looks today.
Unit tests should not be removed and we should make sure we have a unit tests that validates ValidateTaskResultsVariables (which we most likely already have).
There was a problem hiding this comment.
@afrittoli I think you're right to add ValidateStepActionResultsVariables that validates step results only because it has only the step results context and hence i removed the snippet of tests there (we dont have the task result context when using stepAction.Validate)
I am also fixing ValidateStepResultsVariables to add task results in the function args so we can validate the step script against both step and task result because we have them in context as this method is now only called in task validation context
There was a problem hiding this comment.
@afrittoli Can you have a look to my comment pleae
There was a problem hiding this comment.
@jkhelil Sorry about the late reply.
The proposed fix is not ok. Quoting my previous comment:
The change you proposed modifies code used for validation of
StepActions, which is not ok
The validation of StepActions should remain the same and its tests should also remain the same as they are correct. The problem stems from the fact that the Tasks validation function is also used to validate StepActions and it enforces validation that is only required for StepActions.
A correct fix would be to have two different validation functions:
ValidateStepActionResultsVariables, which should look like whatValidateStepResultsVariableslooks today (before this PR)ValidateStepResultsVariables, which should be fixed, like you have done in this PR, to enforce validation that is appropriate for Tasks
If you did that, then you would not need to remove any unit tests.
tekton-robot
added
the
do-not-merge/hold
jkhelil
force-pushed
the
fix_validateTaskResultsVariables
branch
from
5a87c4a to
49fb44c
Compare
tekton-robot
added
size/L
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
jkhelil
force-pushed
the
fix_validateTaskResultsVariables
branch
from
49fb44c to
1a958c3
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
jkhelil
force-pushed
the
fix_validateTaskResultsVariables
branch
from
1a958c3 to
cf117f2
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
…nd fix ValidateStepResultsVariables to include task results validation
jkhelil
force-pushed
the
fix_validateTaskResultsVariables
branch
from
cf117f2 to
43fd366
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
Changes
fixes #8255
There is a mixture of validating step result and task result in both ValidateStepResultsVariables
ValidateStepResultsVariables should use step results set of name to only validate StepResults. TaskResults are validated via the function ValidateTaskResultsVariables
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes