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.

Sign up for GitHub

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

Jump to bottom

[TEP-0075] Extract out the validation of object keys #4867

Merged
merged 1 commit into from
Jun 7, 2022
Merged

[TEP-0075] Extract out the validation of object keys #4867

merged 1 commit into from
Jun 7, 2022

Conversation

chuangw6
Copy link
Member

@chuangw6 chuangw6 commented May 12, 2022 •
edited by afrittoli
Loading

Changes

Prior to this commit, the validation against the required and
provided keys for object type was part of the validateObjectUsage
function. That makes the purpose of validateObjectUsage confusing.

In this commit, a new helper function is created to validate that
separately.

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

NONE

/kind misc

@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 May 12, 2022
@chuangw6 chuangw6 force-pushed the move-out-default-validation branch from 63918b0 to 76d2632 Compare May 12, 2022 21:44
@chuangw6 chuangw6 changed the title Extract out the validation of object keys TEP-0075: Extract out the validation of object keys May 12, 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/task_validation.go 97.6% 97.7% 0.1

@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/task_validation.go 97.6% 97.7% 0.1

@chuangw6 chuangw6 force-pushed the move-out-default-validation branch from 76d2632 to eb9e0a9 Compare May 12, 2022 21:51
@tekton-robot tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 12, 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/task_validation.go 97.6% 97.7% 0.1

@chuangw6 chuangw6 force-pushed the move-out-default-validation branch from eb9e0a9 to 502f9fe Compare May 12, 2022 22:09
@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/task_validation.go 97.6% 97.7% 0.1

@chuangw6 chuangw6 force-pushed the move-out-default-validation branch from 502f9fe to 6688279 Compare May 15, 2022 03:18
@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/task_validation.go 97.6% 97.7% 0.1

@ywluogg
Copy link
Contributor

ywluogg commented May 16, 2022

/assign ywluogg

@chuangw6 chuangw6 changed the title TEP-0075: Extract out the validation of object keys [TEP-0075]: Extract out the validation of object keys May 16, 2022
@chuangw6 chuangw6 changed the title [TEP-0075]: Extract out the validation of object keys [TEP-0075] Extract out the validation of object keys May 16, 2022
ywluogg
ywluogg approved these changes May 16, 2022
View reviewed changes
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.

Thanks for refactoring! LGTM mostly except for the open comment

pkg/apis/pipeline/v1beta1/task_validation.go Outdated Show resolved Hide resolved
@chuangw6 chuangw6 force-pushed the move-out-default-validation branch from 6688279 to 839cff2 Compare May 17, 2022 03:48
@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/task_validation.go 97.7% 97.7% 0.1

@chuangw6
Copy link
Member Author

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

1 similar comment
@chuangw6
Copy link
Member Author

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

@chuangw6 chuangw6 mentioned this pull request May 18, 2022
Merged
5 tasks
@ywluogg
Copy link
Contributor

ywluogg commented May 19, 2022

/assign afrittoli

@chuangw6 chuangw6 force-pushed the move-out-default-validation branch from 839cff2 to 3698987 Compare May 24, 2022 21:59
@chuangw6
Copy link
Member Author

/test pull-tekton-pipeline-integration-tests

@chuangw6
Copy link
Member Author

/retest

@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/task_validation.go 97.7% 97.7% 0.1

@chuangw6
Copy link
Member Author

/test pull-tekton-pipeline-integration-tests

@ywluogg
Copy link
Contributor

ywluogg commented May 25, 2022

/lgtm

@tekton-robot
Copy link
Collaborator

@ywluogg: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

@ywluogg
Copy link
Contributor

ywluogg commented May 26, 2022

/assign @jerop

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2022
@chuangw6
TEP-0075: Extract out the validation of object keys
c2c3272 
Prior to this commit, the validation against the required and
provided keys for object type was part of the validateObjectUsage
function. That makes the purpose of validateObjectUsage confusing.

In this commit, a new helper function is created to validate that
separately.
@chuangw6 chuangw6 force-pushed the move-out-default-validation branch from 3698987 to c2c3272 Compare May 27, 2022 17:13
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 27, 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/task_validation.go 97.7% 97.8% 0.1

ywluogg
ywluogg approved these changes Jun 2, 2022
View reviewed changes
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.

Nice job Chuang!

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester, 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 Jun 2, 2022
@ywluogg
Copy link
Contributor

ywluogg commented Jun 2, 2022

Link this to #4723

@abayer
Copy link
Contributor

abayer commented Jun 7, 2022

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 7, 2022
@tekton-robot tekton-robot merged commit 11f5d57 into tektoncd:main Jun 7, 2022
@tekton-robot tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. kind/misc Categorizes issue or PR as a miscellaneuous one. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 21, 2022
@ywluogg ywluogg mentioned this pull request Jun 22, 2022
Merged
@Yongxuanzhang Yongxuanzhang mentioned this pull request Sep 27, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vdemeester vdemeester vdemeester approved these changes

@ywluogg ywluogg ywluogg approved these changes

@afrittoli afrittoli Awaiting requested review from afrittoli

@dlorenc dlorenc Awaiting requested review from dlorenc

Assignees

@abayer abayer

@afrittoli afrittoli

@jerop jerop

@ywluogg ywluogg

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/misc Categorizes issue or PR as a miscellaneuous one. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesnt merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@chuangw6 @tekton-robot @ywluogg @abayer @vdemeester @afrittoli @jerop