Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TEP-0075] Validate task result variable of object type #4878

Merged

Conversation

chuangw6
Copy link
Member

@chuangw6 chuangw6 commented May 16, 2022

Changes

Prior to this commit, when providing param value with task result
variable, it only allowed using the task result as whole in the format
of tasks.<taskName>.results.<resultName> since result can
be only of type string previously.

As we are adding support for object type result, we need to support
using the result variable of object type in the format of
tasks.<taskName>.results.<objectResultName>.<individualAttribute>.

In this commit, we consider the object case in the validation webhook.

Submitter Checklist

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

  • Docs included if any changes are user facing
  • Tests included if any functionality added or changed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including
    functionality, content, code)
  • Release notes block below has been filled in
    (if there are no user facing changes, use release note "NONE")

Release Notes

add validation and parsing for object result references
For example, $(tasks.mytask.results.myObjectResult[*]) and `$(tasks.mytask.results.myObjectResult.myKey)` 
are valid references to whole object result and a specific key of an object result.

@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 16, 2022
@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 16, 2022
@chuangw6 chuangw6 changed the title [TEP-0075] Validate object task result variable [TEP-0075] Validate task result variable of object type May 16, 2022
@chuangw6 chuangw6 force-pushed the tep75-pipeline-validation-webhook branch 2 times, most recently from 5a4e833 to 9eb408b Compare May 16, 2022 18:22
@chuangw6
Copy link
Member Author

/test tekton-pipeline-unit-tests

@chuangw6 chuangw6 force-pushed the tep75-pipeline-validation-webhook branch from 9eb408b to e83b670 Compare May 16, 2022 18:36
@chuangw6
Copy link
Member Author

/test pull-tekton-pipeline-go-coverage

pkg/apis/pipeline/v1beta1/resultref.go Outdated Show resolved Hide resolved
pkg/apis/pipeline/v1beta1/resultref_test.go Show resolved Hide resolved
pkg/apis/pipeline/v1beta1/resultref_test.go Outdated Show resolved Hide resolved
pkg/apis/pipeline/v1beta1/pipeline_validation_test.go Outdated Show resolved Hide resolved
pkg/apis/pipeline/v1beta1/resultref.go Show resolved Hide resolved
@chuangw6 chuangw6 force-pushed the tep75-pipeline-validation-webhook branch from e83b670 to aacaa37 Compare May 18, 2022 04:05
@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 18, 2022
@chuangw6 chuangw6 force-pushed the tep75-pipeline-validation-webhook branch 2 times, most recently from da753b0 to 032f28b Compare May 18, 2022 04:11
pkg/apis/pipeline/v1beta1/pipeline_validation_test.go Outdated Show resolved Hide resolved
pkg/apis/pipeline/v1beta1/resultref.go Show resolved Hide resolved
pkg/apis/pipeline/v1beta1/resultref.go Outdated Show resolved Hide resolved
pkg/apis/pipeline/v1beta1/resultref.go Show resolved Hide resolved
@chuangw6 chuangw6 force-pushed the tep75-pipeline-validation-webhook branch 2 times, most recently from 9aa60c1 to f55b58e Compare May 24, 2022 22:51
@ywluogg
Copy link
Contributor

ywluogg commented May 26, 2022

/assign ywluogg

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 27, 2022
Copy link
Contributor

@ywluogg ywluogg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM except for another test case to be added

pkg/apis/pipeline/v1beta1/resultref.go Outdated Show resolved Hide resolved
pkg/apis/pipeline/v1beta1/resultref.go Outdated Show resolved Hide resolved
@chuangw6 chuangw6 force-pushed the tep75-pipeline-validation-webhook branch from f55b58e to d16d410 Compare June 2, 2022 18:03
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lbernick, ywluogg

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ywluogg
Copy link
Contributor

ywluogg commented Jun 2, 2022

Link this to #4723

@abayer
Copy link
Contributor

abayer commented Jun 7, 2022

This needs a release note.

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2022
Prior to this commit, when providing param value with task result
variable, it only allowed using the task result as whole in the format
of `tasks.<taskName>.results.<resultName>` since result can
be only of type string previously.

As we are adding support for object type result, we need to support
using the result variable of object type in the format of
`tasks.<taskName>.results.<objectResultName>.<individualAttribute>`.

In this commit, we consider the object case in the validation webhook.
@chuangw6 chuangw6 force-pushed the tep75-pipeline-validation-webhook branch from d16d410 to 0facd8d Compare June 9, 2022 21:29
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2022
@dibyom
Copy link
Member

dibyom commented Jun 13, 2022

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 13, 2022
@chuangw6
Copy link
Member Author

/test tekton-pipeline-unit-tests

@abayer
Copy link
Contributor

abayer commented Jun 13, 2022

/retest

@tekton-robot tekton-robot merged commit df98103 into tektoncd:main Jun 13, 2022
@afrittoli
Copy link
Member

/kind feature

@tekton-robot tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants