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] Support Dictionary in Params #4786

Merged
merged 1 commit into from
May 11, 2022

Conversation

chuangw6
Copy link
Member

@chuangw6 chuangw6 commented Apr 22, 2022

Changes

/kind feature

Note: This new feature is gated by the alpha flag.

This PR serves as an initial implementation of [TEP-0075] which is part of #4723 . Highlights:

Add dictionary type

  • Add properties section in ParamSpec struct (key:string, val: PropertySpec struct)
  • Add ObjectVal in ArrayOrString struct (needs to rename this struct in future pr)
  • Implement UnmarshalJSON and MarshalJSON for ArrayOrString of object type.

Mutating webhook (SetDefault)

  • Infer ParamSpec type if properties section is provided
  • Infer ParamSpec type if objectVal is provided in default
  • Set default type (string) for PropertySpec if properties is provided and PropertySpec's type field is not provided.

Task/TaskRun level Validation

  • ParamSpec Type validation (for object type)
  • PropertySpec Type validation
  • Object type param definition validation i.e. properties is provided, required keys are provided etc.
  • Object param's names and its key names are referenced correctly

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 support for dictionary in Params values (and Results as well). This should affect *backward* compatibilities *but* it will break forward compatibilities in some cases (a.k.a. old client with new pipeline version)
action required: This changes the `ArrayOrStruct` structure, which will have effect on project using the go API as a library.

@tekton-robot tekton-robot added 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. labels Apr 22, 2022
@chuangw6 chuangw6 marked this pull request as draft April 22, 2022 05:03
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/param_types.go 98.1% 96.6% -1.6
pkg/apis/pipeline/v1beta1/task_validation.go 97.1% 90.9% -6.2
pkg/reconciler/pipelinerun/resources/apply.go 100.0% 96.3% -3.7
pkg/reconciler/taskrun/resources/apply.go 99.2% 96.2% -3.0

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/param_types.go 98.1% 96.6% -1.6
pkg/apis/pipeline/v1beta1/task_validation.go 97.1% 90.9% -6.2
pkg/reconciler/pipelinerun/resources/apply.go 100.0% 96.3% -3.7
pkg/reconciler/taskrun/resources/apply.go 99.2% 96.2% -3.0

@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 25, 2022
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/param_types.go 98.1% 94.9% -3.2
pkg/apis/pipeline/v1beta1/task_validation.go 97.1% 90.9% -6.2
pkg/reconciler/pipelinerun/resources/apply.go 100.0% 96.3% -3.7
pkg/reconciler/taskrun/resources/apply.go 99.2% 96.2% -3.0
pkg/reconciler/taskrun/validate_resources.go 95.7% 96.4% 0.7

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/param_types.go 98.1% 94.9% -3.2
pkg/apis/pipeline/v1beta1/task_validation.go 97.1% 90.9% -6.2
pkg/reconciler/pipelinerun/resources/apply.go 100.0% 96.3% -3.7
pkg/reconciler/taskrun/resources/apply.go 99.2% 96.2% -3.0
pkg/reconciler/taskrun/validate_resources.go 95.7% 96.4% 0.7

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/param_types.go 98.1% 96.6% -1.5
pkg/apis/pipeline/v1beta1/task_validation.go 97.1% 97.4% 0.3
pkg/reconciler/pipelinerun/resources/apply.go 100.0% 96.3% -3.7
pkg/reconciler/taskrun/resources/apply.go 99.2% 96.2% -3.0
pkg/reconciler/taskrun/validate_resources.go 95.7% 96.4% 0.7

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/param_types.go 98.1% 96.6% -1.5
pkg/apis/pipeline/v1beta1/task_validation.go 97.1% 97.4% 0.3
pkg/reconciler/pipelinerun/resources/apply.go 100.0% 96.3% -3.7
pkg/reconciler/taskrun/resources/apply.go 99.2% 96.2% -3.0
pkg/reconciler/taskrun/validate_resources.go 95.7% 96.4% 0.7

@ywluogg
Copy link
Contributor

ywluogg commented Apr 29, 2022

/assign @ywluogg

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/param_types.go 98.1% 98.4% 0.3
pkg/apis/pipeline/v1beta1/task_validation.go 97.0% 96.8% -0.2
pkg/reconciler/pipelinerun/resources/apply.go 100.0% 96.3% -3.7
pkg/reconciler/taskrun/resources/apply.go 99.2% 96.2% -3.0
pkg/reconciler/taskrun/validate_resources.go 95.7% 96.4% 0.7

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/param_types.go 98.1% 98.4% 0.3
pkg/apis/pipeline/v1beta1/task_validation.go 97.0% 96.8% -0.2
pkg/reconciler/pipelinerun/resources/apply.go 100.0% 96.3% -3.7
pkg/reconciler/taskrun/resources/apply.go 99.2% 96.2% -3.0
pkg/reconciler/taskrun/validate_resources.go 95.7% 96.4% 0.7

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/param_context.go 96.3% 94.6% -1.7
pkg/apis/pipeline/v1beta1/param_types.go 98.1% 98.4% 0.3
pkg/apis/pipeline/v1beta1/task_validation.go 97.0% 96.8% -0.2
pkg/reconciler/pipelinerun/resources/apply.go 100.0% 96.3% -3.7
pkg/reconciler/taskrun/resources/apply.go 99.2% 96.2% -3.0
pkg/reconciler/taskrun/validate_resources.go 95.7% 96.4% 0.7

@tekton-robot tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 1, 2022
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/param_context.go 96.3% 95.1% -1.2
pkg/apis/pipeline/v1beta1/param_types.go 98.1% 98.5% 0.3
pkg/apis/pipeline/v1beta1/task_validation.go 97.0% 96.8% -0.2
pkg/reconciler/pipelinerun/resources/apply.go 100.0% 96.3% -3.7
pkg/reconciler/taskrun/resources/apply.go 99.2% 96.2% -3.0
pkg/reconciler/taskrun/validate_resources.go 95.7% 96.4% 0.7

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/param_context.go 96.3% 95.1% -1.2
pkg/apis/pipeline/v1beta1/param_types.go 98.1% 98.5% 0.3
pkg/apis/pipeline/v1beta1/task_validation.go 97.0% 96.8% -0.2
pkg/reconciler/pipelinerun/resources/apply.go 100.0% 96.3% -3.7
pkg/reconciler/taskrun/resources/apply.go 99.2% 96.2% -3.0
pkg/reconciler/taskrun/validate_resources.go 95.7% 96.4% 0.7

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/param_context.go 96.3% 95.1% -1.2
pkg/apis/pipeline/v1beta1/param_types.go 98.1% 98.5% 0.3
pkg/apis/pipeline/v1beta1/task_validation.go 97.0% 96.8% -0.2
pkg/reconciler/pipelinerun/resources/apply.go 100.0% 96.3% -3.7
pkg/reconciler/taskrun/resources/apply.go 99.2% 96.2% -3.0
pkg/reconciler/taskrun/validate_resources.go 95.7% 96.4% 0.7

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/param_context.go 96.3% 95.1% -1.2
pkg/apis/pipeline/v1beta1/param_types.go 98.1% 98.5% 0.3
pkg/apis/pipeline/v1beta1/task_validation.go 97.0% 96.8% -0.2
pkg/reconciler/taskrun/validate_resources.go 95.7% 96.4% 0.7

@chuangw6 chuangw6 marked this pull request as ready for review May 1, 2022 23:53
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label May 10, 2022
@chuangw6
Copy link
Member Author

/retest

@chuangw6
Copy link
Member Author

/test pull-tekton-pipeline-go-coverage

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/param_context.go 96.3% 94.7% -1.6
pkg/apis/pipeline/v1beta1/param_types.go 98.1% 96.9% -1.3
pkg/apis/pipeline/v1beta1/task_validation.go 97.0% 97.5% 0.5
pkg/reconciler/taskrun/validate_resources.go 96.0% 95.9% -0.1

@chuangw6
Copy link
Member Author

/test pull-tekton-pipeline-integration-tests

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/param_context.go 96.3% 94.7% -1.6
pkg/apis/pipeline/v1beta1/param_types.go 98.1% 96.9% -1.3
pkg/apis/pipeline/v1beta1/task_validation.go 97.0% 97.5% 0.5
pkg/reconciler/taskrun/validate_resources.go 95.3% 93.7% -1.6

@chuangw6
Copy link
Member Author

/test pull-tekton-pipeline-alpha-integration-tests

Copy link
Member

@dibyom dibyom left a comment

Choose a reason for hiding this comment

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

Thanks @chuangw6

Have a couple of minor comments around TODOs but otherwise LGTM!

pkg/apis/pipeline/v1beta1/param_types.go Outdated Show resolved Hide resolved
pkg/apis/pipeline/v1beta1/task_validation.go Outdated Show resolved Hide resolved
pkg/reconciler/taskrun/validate_resources.go Outdated Show resolved Hide resolved
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom, 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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 11, 2022
Note: This new feature is gated by the alpha flag.
Changes:
- Add `properties` section in ParamSpec struct
- Add `ObjectVal` in ArrayOrString struct
- Implement UnmarshalJSON and MarshalJSON for ArrayOrString of object type.
- Change SetDefault function to include object type
- Add type validation for object type in task_validation.go
- Add object param validation in validate_resources.go
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/param_context.go 96.3% 94.7% -1.6
pkg/apis/pipeline/v1beta1/param_types.go 98.1% 96.9% -1.3
pkg/apis/pipeline/v1beta1/task_validation.go 97.0% 97.5% 0.5
pkg/reconciler/taskrun/validate_resources.go 95.3% 93.7% -1.6

@dibyom
Copy link
Member

dibyom commented May 11, 2022

/lgtm

All comments have been addressed

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 11, 2022
@tekton-robot tekton-robot merged commit 1c8447d into tektoncd:main May 11, 2022
@pritidesai
Copy link
Member

I am sorry to bring this up @wlynch @dibyom 🙏

This PR is introducing a new feature and was merged without release notes. Please make sure we check the PR description before its merged!

@tekton-robot tekton-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 1, 2022
@vdemeester
Copy link
Member

I've updated the description to add a release-note block.

@chuangw6
Copy link
Member Author

chuangw6 commented Jun 1, 2022

I've updated the description to add a release-note block.

Thanks @vdemeester !

This PR is introducing a new feature and was merged without release notes. Please make sure we check the PR description before its merged!

Thanks! @pritidesai I'll keep in mind to include release note for new features added in future.

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-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants