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

Migrate core interceptors to new format #1029

Merged
merged 1 commit into from
Apr 7, 2021
+256 −16
Merged

Migrate core interceptors to new format #1029

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions pkg/apis/triggers/v1alpha1/event_listener_defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package v1alpha1
import (
"context"

"knative.dev/pkg/logging"
"knative.dev/pkg/ptr"
)

Expand All @@ -36,6 +37,12 @@ func (el *EventListener) SetDefaults(ctx context.Context) {
triggerSpecBindingArray(el.Spec.Triggers[i].Bindings).defaultBindings()
for _, ti := range t.Interceptors {
ti.defaultInterceptorKind()
if err := ti.updateCoreInterceptors(); err != nil {
// The err only happens due to malformed JSON and should never really happen
// We can't return an error here, so print out the error
logger := logging.FromContext(ctx)
logger.Errorf("failed to setDefaults for trigger: %s; err: %s", t.Name, err)
}
}
}
// Remove Deprecated Resource Fields
Expand Down
8 changes: 8 additions & 0 deletions pkg/apis/triggers/v1alpha1/trigger_defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ package v1alpha1

import (
"context"

"knative.dev/pkg/logging"
)

type triggerSpecBindingArray []*TriggerSpecBinding
Expand All @@ -30,6 +32,12 @@ func (t *Trigger) SetDefaults(ctx context.Context) {
triggerSpecBindingArray(t.Spec.Bindings).defaultBindings()
for _, ti := range t.Spec.Interceptors {
ti.defaultInterceptorKind()
if err := ti.updateCoreInterceptors(); err != nil {
// The err only happens due to malformed JSON and should never really happen
// We can't return an error here, so print out the error
logger := logging.FromContext(ctx)
logger.Errorf("failed to setDefaults for trigger: %s; err: %s", t.Name, err)
}
}
}

Expand Down
145 changes: 145 additions & 0 deletions pkg/apis/triggers/v1alpha1/trigger_defaults_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (

"github.com/google/go-cmp/cmp"
"github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1"
"github.com/tektoncd/triggers/test"
)

func TestTriggerSetDefaults(t *testing.T) {
Expand Down Expand Up @@ -98,6 +99,150 @@ func TestTriggerSetDefaults(t *testing.T) {
}},
},
},
}, {
name: "default deprecatedGithub to new ref/params",
in: &v1alpha1.Trigger{
Spec: v1alpha1.TriggerSpec{
Interceptors: []*v1alpha1.TriggerInterceptor{{
DeprecatedGitHub: &v1alpha1.GitHubInterceptor{
SecretRef: &v1alpha1.SecretRef{
SecretKey: "key",
SecretName: "name",
},
EventTypes: []string{"push"},
},
}},
},
},
wc: v1alpha1.WithUpgradeViaDefaulting,
want: &v1alpha1.Trigger{
Spec: v1alpha1.TriggerSpec{
Interceptors: []*v1alpha1.TriggerInterceptor{{
Ref: v1alpha1.InterceptorRef{
Name: "github",
Kind: v1alpha1.ClusterInterceptorKind,
},
Params: []v1alpha1.InterceptorParams{{
Name: "secretRef",
Value: test.ToV1JSON(t, &v1alpha1.SecretRef{
SecretKey: "key",
SecretName: "name",
}),
}, {
Name: "eventTypes",
Value: test.ToV1JSON(t, []string{"push"}),
}},
}},
},
},
}, {
name: "default deprecatedGitlab to new ref/params",
in: &v1alpha1.Trigger{
Spec: v1alpha1.TriggerSpec{
Interceptors: []*v1alpha1.TriggerInterceptor{{
DeprecatedGitLab: &v1alpha1.GitLabInterceptor{
SecretRef: &v1alpha1.SecretRef{
SecretKey: "key",
SecretName: "name",
},
EventTypes: []string{"push"},
},
}},
},
},
wc: v1alpha1.WithUpgradeViaDefaulting,
want: &v1alpha1.Trigger{
Spec: v1alpha1.TriggerSpec{
Interceptors: []*v1alpha1.TriggerInterceptor{{
Ref: v1alpha1.InterceptorRef{
Name: "gitlab",
Kind: v1alpha1.ClusterInterceptorKind,
},
Params: []v1alpha1.InterceptorParams{{
Name: "secretRef",
Value: test.ToV1JSON(t, &v1alpha1.SecretRef{
SecretKey: "key",
SecretName: "name",
}),
}, {
Name: "eventTypes",
Value: test.ToV1JSON(t, []string{"push"}),
}},
}},
},
},
}, {
name: "default deprecatedBitbucket to new ref/params",
in: &v1alpha1.Trigger{
Spec: v1alpha1.TriggerSpec{
Interceptors: []*v1alpha1.TriggerInterceptor{{
DeprecatedBitbucket: &v1alpha1.BitbucketInterceptor{
SecretRef: &v1alpha1.SecretRef{
SecretKey: "key",
SecretName: "name",
},
EventTypes: []string{"push"},
},
}},
},
},
wc: v1alpha1.WithUpgradeViaDefaulting,
want: &v1alpha1.Trigger{
Spec: v1alpha1.TriggerSpec{
Interceptors: []*v1alpha1.TriggerInterceptor{{
Ref: v1alpha1.InterceptorRef{
Name: "bitbucket",
Kind: v1alpha1.ClusterInterceptorKind,
},
Params: []v1alpha1.InterceptorParams{{
Name: "secretRef",
Value: test.ToV1JSON(t, &v1alpha1.SecretRef{
SecretKey: "key",
SecretName: "name",
}),
}, {
Name: "eventTypes",
Value: test.ToV1JSON(t, []string{"push"}),
}},
}},
},
},
}, {
name: "default deprecatedCEL to new ref/params",
in: &v1alpha1.Trigger{
Spec: v1alpha1.TriggerSpec{
Interceptors: []*v1alpha1.TriggerInterceptor{{
DeprecatedCEL: &v1alpha1.CELInterceptor{
Filter: "body.foo == bar",
Overlays: []v1alpha1.CELOverlay{{
Key: "abc",
Expression: "body.foo",
}},
},
}},
},
},
wc: v1alpha1.WithUpgradeViaDefaulting,
want: &v1alpha1.Trigger{
Spec: v1alpha1.TriggerSpec{
Interceptors: []*v1alpha1.TriggerInterceptor{{
Ref: v1alpha1.InterceptorRef{
Name: "cel",
Kind: v1alpha1.ClusterInterceptorKind,
},
Params: []v1alpha1.InterceptorParams{{
Name: "filter",
Value: test.ToV1JSON(t, "body.foo == bar"),
}, {
Name: "overlays",
Value: test.ToV1JSON(t, []v1alpha1.CELOverlay{{
Key: "abc",
Expression: "body.foo",
}}),
}},
}},
},
},
}}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
Expand Down
74 changes: 74 additions & 0 deletions pkg/apis/triggers/v1alpha1/trigger_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ limitations under the License.
package v1alpha1

import (
"encoding/json"
"fmt"

"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
corev1 "k8s.io/api/core/v1"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
Expand Down Expand Up @@ -135,6 +138,77 @@ func (ti *TriggerInterceptor) defaultInterceptorKind() {
}
}

func (ti *TriggerInterceptor) updateCoreInterceptors() error {
if ti == nil {
return nil
}
if ti.Ref.Name != "" {
return nil
}
ti.Ref.Name = ti.GetName()
ti.Params = []InterceptorParams{}
switch ti.Ref.Name {
case "bitbucket":
if err := addToParams(&ti.Params, "secretRef", ti.DeprecatedBitbucket.SecretRef); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

These could nil panic if ti or DeprecatedBitbucket is nil. We should add checks to prevent this (as well as for the other fields)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a check for when ti is nil.
A bit convoluted but the checks for the other fields being not nil happens in the call ti.GetName() which ensures that if the name is bitbucket, then ti.DeprecatedBitbucket is not nil: https://github.com/tektoncd/triggers/blob/main/pkg/apis/triggers/v1alpha1/trigger_types.go#L145

return err
}
if err := addToParams(&ti.Params, "eventTypes", ti.DeprecatedBitbucket.EventTypes); err != nil {
return err
}
ti.DeprecatedBitbucket = nil
case "gitlab":
if err := addToParams(&ti.Params, "secretRef", ti.DeprecatedGitLab.SecretRef); err != nil {
return err
}
if err := addToParams(&ti.Params, "eventTypes", ti.DeprecatedGitLab.EventTypes); err != nil {
return err
}
ti.DeprecatedGitLab = nil
case "github":
if err := addToParams(&ti.Params, "secretRef", ti.DeprecatedGitHub.SecretRef); err != nil {
return err
}
if err := addToParams(&ti.Params, "eventTypes", ti.DeprecatedGitHub.EventTypes); err != nil {
return err
}
ti.DeprecatedGitHub = nil
case "cel":
if err := addToParams(&ti.Params, "filter", ti.DeprecatedCEL.Filter); err != nil {
return err
}
if err := addToParams(&ti.Params, "overlays", ti.DeprecatedCEL.Overlays); err != nil {
return err
}
ti.DeprecatedCEL = nil
}
return nil
}

func addToParams(params *[]InterceptorParams, name string, val interface{}) error {
if val == nil {
return nil
}
v, err := toV1JSON(val)
if err != nil {
return err
}
*params = append(*params, InterceptorParams{
Name: name,
Value: v,
})
return nil
}

func toV1JSON(v interface{}) (apiextensionsv1.JSON, error) {
b, err := json.Marshal(v)
if err != nil {
return apiextensionsv1.JSON{}, fmt.Errorf("json.Marshal() failed: %s", err)
}
return apiextensionsv1.JSON{
Raw: b,
}, nil
}

// GetName returns the name for the given interceptor
func (ti *TriggerInterceptor) GetName() string {
// This is temporary until we implement #869
Expand Down
38 changes: 22 additions & 16 deletions pkg/apis/triggers/v1alpha1/trigger_types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,46 +14,44 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package v1alpha1_test
package v1alpha1

import (
"testing"

"github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1"
)

func TestGetName(t *testing.T) {
for _, tc := range []struct {
in v1alpha1.TriggerInterceptor
in TriggerInterceptor
want string
}{{
in: v1alpha1.TriggerInterceptor{
DeprecatedCEL: &v1alpha1.CELInterceptor{},
in: TriggerInterceptor{
DeprecatedCEL: &CELInterceptor{},
},
want: "cel",
}, {
in: v1alpha1.TriggerInterceptor{
DeprecatedGitLab: &v1alpha1.GitLabInterceptor{},
in: TriggerInterceptor{
DeprecatedGitLab: &GitLabInterceptor{},
},
want: "gitlab",
}, {
in: v1alpha1.TriggerInterceptor{
DeprecatedGitHub: &v1alpha1.GitHubInterceptor{},
in: TriggerInterceptor{
DeprecatedGitHub: &GitHubInterceptor{},
},
want: "github",
}, {
in: v1alpha1.TriggerInterceptor{
DeprecatedBitbucket: &v1alpha1.BitbucketInterceptor{},
in: TriggerInterceptor{
DeprecatedBitbucket: &BitbucketInterceptor{},
},
want: "bitbucket",
}, {
in: v1alpha1.TriggerInterceptor{
Webhook: &v1alpha1.WebhookInterceptor{},
in: TriggerInterceptor{
Webhook: &WebhookInterceptor{},
},
want: "",
}, {
in: v1alpha1.TriggerInterceptor{
Ref: v1alpha1.InterceptorRef{
in: TriggerInterceptor{
Ref: InterceptorRef{
Name: "pluggable-interceptor",
},
},
Expand All @@ -67,3 +65,11 @@ func TestGetName(t *testing.T) {
})
}
}

func TestUpdateCoreInterceptors_Error(t *testing.T) {
var ti *TriggerInterceptor

if err := ti.updateCoreInterceptors(); err != nil {
t.Fatalf("updateCoreInterceptors() unexpected error: %s", err)
}
}