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

Clean up validation for task and pipeline refs #5046

Merged
merged 1 commit into from
Jul 7, 2022
Merged

Clean up validation for task and pipeline refs #5046

merged 1 commit into from
Jul 7, 2022

Conversation

lbernick
Copy link
Member

Changes

This commit creates separate files for testing validation of Task and Pipeline
references, and moves test cases related to this validation into those files.
It also updates the error message for when a bundle is included in a reference
without the "enable-tekton-oci-bundles" flag set to "true" to instruct the user
to set that flag. In addition, it updates the error message for when resolvers
are set without the "enable-api-fields" flag set to "alpha" to instruct the user
to set that flag.

/kind cleanup

Submitter Checklist

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

  • n/a 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

@tekton-robot tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Jun 27, 2022
@tekton-robot tekton-robot requested review from dlorenc and jerop June 27, 2022 18:13
@tekton-robot tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 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/pipelineref_validation.go 96.8% 100.0% 3.2
pkg/apis/pipeline/v1beta1/taskref_validation.go 97.6% 95.0% -2.6

@abayer
Copy link
Contributor

abayer commented Jun 27, 2022

/approve

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

thanks @lbernick I was just looking at the v1beta1 package and a list of files we have in there - about 80 😲

I am looking at the best practices of how to refactor this.

If we desire to go with a single flat API package, it will be cleaner to have:

  • _types.go
  • _types_test.go
  • _defaults.go
  • _defaults_test.go
  • _validation.go
  • _validation_test.go

We can follow this pattern for any type including Task, TaskRef, TaskSpec, etc.

thoughts?

@pritidesai
Copy link
Member

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/pipelineref_validation.go 96.8% 100.0% 3.2
pkg/apis/pipeline/v1beta1/taskref_validation.go 97.6% 95.0% -2.6

Refactoring is bringing down the coverage, please address this 🙏

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 28, 2022
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 28, 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/pipelineref_validation.go 96.8% 100.0% 3.2
pkg/apis/pipeline/v1beta1/taskref_validation.go 97.6% 95.0% -2.6

@lbernick
Copy link
Member Author

Not sure what is going on here: tests are failing with

$ git merge --no-ff 0692a5fce64a8d24b0bf60680cf2ca457ae6e186
merge: 0692a5fce64a8d24b0bf60680cf2ca457ae6e186 - not something we can merge

but they pass locally?

/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/pipelineref_validation.go 96.8% 100.0% 3.2
pkg/apis/pipeline/v1beta1/taskref_validation.go 97.6% 95.0% -2.6

@lbernick
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/pipelineref_validation.go 96.8% 100.0% 3.2
pkg/apis/pipeline/v1beta1/taskref_validation.go 97.6% 95.0% -2.6

@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/pipelineref_validation.go 96.8% 100.0% 3.2
pkg/apis/pipeline/v1beta1/taskref_validation.go 97.6% 100.0% 2.4

@lbernick
Copy link
Member Author

@pritidesai I've brought up the coverage and moved taskrefs + pipelinerefs into _types.go files. (No need right now for _defaults.go or _types_test.go files IMO)

@pritidesai
Copy link
Member

/retest

1 similar comment
@lbernick
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/pipelineref_validation.go 96.8% 100.0% 3.2
pkg/apis/pipeline/v1beta1/taskref_validation.go 97.6% 100.0% 2.4

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abayer, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@lbernick lbernick force-pushed the ref branch 2 times, most recently from 9f2e483 to b3b623a Compare July 7, 2022 18:02
@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/pipelineref_validation.go 96.8% 100.0% 3.2
pkg/apis/pipeline/v1beta1/taskref_validation.go 97.6% 100.0% 2.4

@lbernick
Clean up validation for task and pipeline refs
19d0185 
This commit creates separate files for testing validation of Task and Pipeline
references, and moves test cases related to this validation into those files.
It also updates the error message for when a bundle is included in a reference
without the "enable-tekton-oci-bundles" flag set to "true" to instruct the user
to set that flag. In addition, it updates the error message for when resolvers
are set without the "enable-api-fields" flag set to "alpha" to instruct the user
to set that flag.
pritidesai
pritidesai reviewed Jul 7, 2022
View reviewed changes
pkg/apis/pipeline/v1beta1/taskref_validation_test.go
@@ -0,0 +1,189 @@
/*
Copyright 2019 The Tekton Authors
Copy link
Member

Choose a reason for hiding this comment

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

NIT: s/2019/2022/

pritidesai
pritidesai reviewed Jul 7, 2022
View reviewed changes
pkg/apis/pipeline/v1beta1/pipelineref_validation_test.go
@@ -0,0 +1,200 @@
/*
Copyright 2020 The Tekton Authors
Copy link
Member

Choose a reason for hiding this comment

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

NIT: s/2020/2022/

@pritidesai
Copy link
Member

thanks a bunch @lbernick 👍

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 7, 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/pipelineref_validation.go 96.8% 100.0% 3.2
pkg/apis/pipeline/v1beta1/taskref_validation.go 97.6% 100.0% 2.4

@tekton-robot tekton-robot merged commit f93ff6f into tektoncd:main Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pritidesai pritidesai pritidesai left review comments

@vdemeester vdemeester vdemeester approved these changes

@dlorenc dlorenc Awaiting requested review from dlorenc

@jerop jerop Awaiting requested review from jerop

Assignees

@pritidesai pritidesai

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesnt merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lbernick @tekton-robot @abayer @pritidesai @vdemeester