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

Add interceptor to processTriggerSpec, update readHTTP and trigger function #695

Merged
merged 1 commit into from
Aug 10, 2020

Conversation

dorismeixing
Copy link
Member

@dorismeixing dorismeixing commented Aug 4, 2020

…esources and trigger function to run Trigger CLI

Changes

This PR is a part of creating CLI for TriggerRun. It contains the parts that add interceptor to processTriggerSpec, updates readHTTP to print out resources and trigger function to run Trigger CLI.
(Related to issue #624. )

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.

Release Notes

NONE

@tekton-robot tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 4, 2020
@dorismeixing
Copy link
Member Author

Add reviewer @dibyom @wlynch
/assign dibyom
/assign wlynch

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.

Make sure to clean up any commented out code!

cmd/triggerRun/cmd/root.go Show resolved Hide resolved
cmd/triggerRun/cmd/root.go Outdated Show resolved Hide resolved
cmd/triggerRun/cmd/root_test.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
examples/gitlab/README.md Outdated Show resolved Hide resolved
cmd/triggerRun/testdata/trigger.yaml Outdated Show resolved Hide resolved
cmd/triggerRun/cmd/root.go Outdated Show resolved Hide resolved
cmd/triggerRun/cmd/root_test.go Outdated Show resolved Hide resolved
@savitaashture
Copy link
Contributor

savitaashture commented Aug 5, 2020

I think mistakenly you have checked in a binary triggerRun as part of the PR Please remove that

@savitaashture
Copy link
Contributor

savitaashture commented Aug 5, 2020

I think we should follow the naming convention for directory name here for cmd/triggerRun to something like cmd/triggerrun or cmd/trigger-run
/cc @dibyom

@dibyom
Copy link
Member

dibyom commented Aug 5, 2020

I think we should follow the naming convention for directory name here for cmd/triggerRun to something like cmd/triggerrun or cmd/trigger-run

yeah I agree cmd/triggerrun sounds fine to me. Though I think that can be a separate PR in order to keep this one contained.

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.

To fix the build tests:

/home/prow/go/src/github.com/tektoncd/triggers is out of date. Please run hack/update-codegen.sh

cmd/triggerrun/cmd/root.go Outdated Show resolved Hide resolved
cmd/triggerrun/cmd/root.go Outdated Show resolved Hide resolved
cmd/triggerrun/cmd/root_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
cmd/triggerrun/cmd/root.go Do not exist 45.7%

@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
cmd/triggerrun/cmd/root.go Do not exist 45.7%

@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
cmd/triggerrun/cmd/root.go Do not exist 44.8%

@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
cmd/triggerrun/cmd/root.go Do not exist 44.8%

@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
cmd/triggerrun/cmd/root.go Do not exist 44.8%

@@ -20,7 +20,8 @@ import (
"fmt"
"os"

"github.com/tektoncd/triggers/cmd/triggerRun/cmd"
"github.com/tektoncd/triggers/cmd/triggerrun/cmd"
_ "k8s.io/client-go/plugin/pkg/client/auth"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_ "k8s.io/client-go/plugin/pkg/client/auth"

Can we remove this import as its not used

Copy link
Member

Choose a reason for hiding this comment

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

We actually need this one. Its contains auth plugins needed to setup the kubeclients for various k8s providers. See https://github.com/kubernetes/client-go/blob/master/examples/out-of-cluster-client-configuration/main.go#L33

e.g. https://github.com/kubernetes/client-go/blob/master/examples/out-of-cluster-client-configuration/main.go#L33

It is an interesting pattern, I'll admit 😛

@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
cmd/triggerrun/cmd/root.go Do not exist 44.8%

@dibyom
Copy link
Member

dibyom commented Aug 7, 2020

Let's also squash up the commits :)

@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
cmd/triggerrun/cmd/root.go Do not exist 44.8%

…esources and trigger function to run Trigger CLI
@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
cmd/triggerrun/cmd/root.go Do not exist 44.8%

@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

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 Aug 7, 2020
@dibyom
Copy link
Member

dibyom commented Aug 10, 2020

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2020
@tekton-robot tekton-robot merged commit 1779f66 into tektoncd:master Aug 10, 2020
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. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants