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

Migrate from test builders to structs #1080

Merged
merged 1 commit into from
May 11, 2021
Merged

Migrate from test builders to structs #1080

merged 1 commit into from
May 11, 2021

Conversation

dibyom
Copy link
Member

@dibyom dibyom commented May 7, 2021 •
edited
Loading

Changes

This is helpful as we migrate to the v1beta1 types since we won't have
to maintain separate test builders for v1beta1 types.

Ref #1067, #735

/kind cleanup
/release-note-none

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes tests (if functionality changed/added)
  • Includes docs (if user facing)
  • Commit messages follow commit message best practices
  • Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Release Notes

@tekton-robot tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels May 7, 2021
@tekton-robot tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 7, 2021
@dibyom
Copy link
Member Author

dibyom commented May 7, 2021

/assign @wlynch

@jmcshane

@tekton-robot
Copy link

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

File Old Coverage New Coverage Delta
pkg/apis/triggers/v1alpha1/event_listener_validation.go 97.8% 97.0% -0.7
pkg/apis/triggers/v1alpha1/trigger_binding_validation.go 100.0% 87.1% -12.9
pkg/apis/triggers/v1alpha1/trigger_validation.go 94.1% 66.2% -27.9

@jmcshane
Copy link
Contributor

jmcshane commented May 7, 2021

Wondering, does this PR allow us to remove any of the existing builders? I feel like moving towards reducing the surface area of the builder functions is the way to go.

@dibyom
Copy link
Member Author

dibyom commented May 7, 2021

Wondering, does this PR allow us to remove any of the existing builders?

Perhaps. We'll have to double check with the CLI folks. They might be using the test builders

wlynch
wlynch reviewed May 7, 2021
View reviewed changes
Copy link
Member

@wlynch wlynch 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! Few minor things.

pkg/apis/triggers/v1alpha1/event_listener_validation_test.go Outdated Show resolved Hide resolved
pkg/apis/triggers/v1alpha1/event_listener_validation_test.go Show resolved Hide resolved
pkg/apis/triggers/v1alpha1/event_listener_validation_test.go Outdated Show resolved Hide resolved
pkg/apis/triggers/v1alpha1/trigger_validation_test.go Outdated Show resolved Hide resolved
pkg/template/event_test.go Outdated Show resolved Hide resolved
@tekton-robot
Copy link

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

File Old Coverage New Coverage Delta
pkg/apis/triggers/v1alpha1/trigger_binding_validation.go 100.0% 87.1% -12.9

@tekton-robot
Copy link

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

File Old Coverage New Coverage Delta
pkg/apis/triggers/v1alpha1/trigger_binding_validation.go 100.0% 96.8% -3.2

@dibyom
Copy link
Member Author

dibyom commented May 7, 2021

/hold

(I missed porting a test case from trigger_binding_Validation.go)

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 7, 2021
@dibyom
Migrate from test builders to structs
9816419 
This is helpful as we migrate to the v1beta1 types since we won't have
to maintain separate test builders for v1beta1 types.

Ref tektoncd#1067, tektoncd#735

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
@dibyom
Copy link
Member Author

dibyom commented May 9, 2021

/hold cancel

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 9, 2021
@dibyom
Copy link
Member Author

dibyom commented May 9, 2021

/test pull-tekton-triggers-go-coverage

This was referenced May 10, 2021
Closed
Merged
@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: savitaashture

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, 2021
@vdemeester
Copy link
Member

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 11, 2021
@tekton-robot tekton-robot merged commit 74dec96 into tektoncd:main May 11, 2021
@dibyom dibyom deleted the remove_builders branch May 11, 2021 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wlynch wlynch wlynch left review comments

@savitaashture savitaashture savitaashture approved these changes

@bobcatfish bobcatfish Awaiting requested review from bobcatfish

@iancoffey iancoffey Awaiting requested review from iancoffey

Assignees

@vdemeester vdemeester

@wlynch wlynch

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 Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dibyom @tekton-robot @jmcshane @vdemeester @wlynch @savitaashture