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

Add String only Params for Triggers #532

Merged
merged 3 commits into from
Apr 29, 2020
Merged

Add String only Params for Triggers #532

merged 3 commits into from
Apr 29, 2020

Conversation

dibyom
Copy link
Member

@dibyom dibyom commented Apr 10, 2020 •
edited
Loading

Changes

This PR is part of #415 and has 3 4 3 commits:

  • Remove pipeline clientsets from the Triggers controller/reconciler. They were not being used anywhere.

  • Create Param and ParamSpec types for Triggers instead of reusing the one from Pipeline. The Triggers version only supports string only params.

  • Fixes a test that was changing a global variable meaning the running tests more than once (-count=2) would always fail.

  • Update third_party/vendor to make build tests pass (moved to Fix license file locations for ko #559)

There should be no user facing changes from this refactoring. There will be a follow up PR to get rid of the remaining uses of Pipelines (in webhook headers and tests)

Submitter Checklist

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

See the contribution guide for more details.

@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 10, 2020
@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 10, 2020
This was referenced Apr 21, 2020
Closed
Closed
@tekton-robot tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 24, 2020
@dibyom dibyom force-pushed the pipeline branch 2 times, most recently from 34487b5 to aef1991 Compare April 24, 2020 22:59
@dibyom dibyom marked this pull request as ready for review April 27, 2020 17:34
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 27, 2020
@dibyom dibyom changed the title Remove unused pipeline clients from Triggers Add String only Params for Triggers Apr 27, 2020
@dibyom
Copy link
Member Author

dibyom commented Apr 27, 2020

@ncskier @wlynch ready for a look

@tekton-robot tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 27, 2020
@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/sink/initialization.go 33.3% 44.4% 11.1

@piyush-garg
Copy link
Contributor

/test pull-tekton-triggers-go-coverage

@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/sink/initialization.go 33.3% 44.4% 11.1

@dibyom
Copy link
Member Author

dibyom commented Apr 28, 2020

/hold

Integration tests are failing due to go modules upgrade changing how vendor licenses are stored. Fixing that in a different PR

@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 Apr 28, 2020
@dibyom dibyom mentioned this pull request Apr 28, 2020
Closed
3 tasks
@dibyom
Copy link
Member Author

dibyom commented Apr 28, 2020

We can cancel the hold once #559 is merged

@tekton-robot tekton-robot removed the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 28, 2020
@tekton-robot tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 28, 2020
@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/sink/initialization.go 33.3% 44.4% 11.1

@dibyom dibyom requested a review from ncskier April 28, 2020 21:24
@dibyom
Copy link
Member Author

dibyom commented Apr 28, 2020

/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 Apr 28, 2020
@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/sink/initialization.go 33.3% 44.4% 11.1

ncskier
ncskier requested changes Apr 28, 2020
View reviewed changes
Copy link
Member

@ncskier ncskier left a comment

Choose a reason for hiding this comment

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

Thanks @dibyom, looks great! Just a few copy & paste things to address 🙂

pkg/apis/triggers/v1alpha1/param.go Outdated Show resolved Hide resolved
pkg/apis/triggers/v1alpha1/param.go Outdated Show resolved Hide resolved
pkg/apis/triggers/v1alpha1/param.go Outdated Show resolved Hide resolved
pkg/apis/triggers/v1alpha1/param.go Show resolved Hide resolved
pkg/logging/logging.go Show resolved Hide resolved
pkg/sink/sink.go Outdated Show resolved Hide resolved
pkg/template/resource_test.go Outdated Show resolved Hide resolved
@dibyom
Add string only Param types for Bindings/Templates
700de90 
Instead of using Pipeline's Param types, just use Triggers' own Param type.
Since we do not use arrayValues for Params in Bindings or Templates, this
Param only supports string values.
@dibyom
Set UID value directly
b923ea0 
We were overriding the UID() function to always return "abcde"
in a test.So, if we ran tests multiple times (e.g. -count=10),
any test that relied on UID() would fail from the second time
onwards
@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/sink/initialization.go 33.3% 44.4% 11.1

ncskier
ncskier approved these changes Apr 29, 2020
View reviewed changes
Copy link
Member

@ncskier ncskier left a comment

Choose a reason for hiding this comment

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

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 29, 2020
@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ncskier

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 Apr 29, 2020
@tekton-robot tekton-robot merged commit 1b76859 into tektoncd:master Apr 29, 2020
@dibyom dibyom deleted the pipeline branch May 5, 2020 15:01
@a-roberts a-roberts mentioned this pull request May 26, 2020
Closed
savitaashture added a commit to savitaashture/triggers that referenced this pull request Aug 31, 2021
@savitaashture
Merge pull request tektoncd#532 from savitaashture/remove-apimachiner…
8fbdc9d 
…y-changes

Remove apimachinery patch changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vdemeester vdemeester vdemeester left review comments

@ncskier ncskier ncskier approved these changes

@iancoffey iancoffey Awaiting requested review from iancoffey

@vtereso vtereso Awaiting requested review from vtereso

Assignees

@ncskier ncskier

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes lgtm Indicates that a PR is ready to be merged. 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.
6 participants
@dibyom @tekton-robot @piyush-garg @vdemeester @ncskier @googlebot