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

Resolve TriggerBindings with no Kind set in sink #461

Merged
merged 1 commit into from
Feb 28, 2020

Conversation

dibyom
Copy link
Member

@dibyom dibyom commented Feb 27, 2020

Changes

When upgrading from 0.2.1 to 0.3, the Sink might have
to process a Binding that has no Kind set. This is
because, we only persist the default Kind when the Binding
object is created/updated. The reconciler also sets the defaults
by calling SetDefaults but that change is not persisted to etcd.

In the future, we might want to consider updating the object from the
reconciler itself if any default values were set.

Fixes: #459

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 size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 27, 2020
@dibyom dibyom requested review from ncskier and akihikokuroda and removed request for iancoffey and dlorenc February 27, 2020 22:18
@dibyom dibyom changed the title Resolve TriggerBindings with no Kind set in sink wip:Resolve TriggerBindings with no Kind set in sink Feb 27, 2020
@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 Feb 27, 2020
@ncskier
Copy link
Member

ncskier commented Feb 27, 2020

/test pull-tekton-triggers-build-tests

@ncskier
Copy link
Member

ncskier commented Feb 27, 2020

This might be a problem if a user does not add the kind field for their bindings, and then tries to update the EventListener, right? https://github.com/tektoncd/triggers/blob/master/pkg/apis/triggers/v1alpha1/event_listener_validation.go#L53

if b.Kind != NamespacedTriggerBindingKind && b.Kind != ClusterTriggerBindingKind {
	return apis.ErrInvalidValue(fmt.Errorf("invalid kind"), fmt.Sprintf("bindings[%d].kind", i))
}

@dibyom
Copy link
Member Author

dibyom commented Feb 27, 2020

This might be a problem if a user does not add the kind field for their bindings, and then tries to update the EventListener, right? https://github.com/tektoncd/triggers/blob/master/pkg/apis/triggers/v1alpha1/event_listener_validation.go#L53

I don't think so. For any newly created/updated bindings, we'll set the right defaults so that validation branch should never be hit. The issue is only for old bindings that are not updated.

@dibyom
Copy link
Member Author

dibyom commented Feb 27, 2020

Also, the build failures are real, still testing this out :D

When upgrading from 0.2.1 to 0.3, the Sink might have
to process a Binding that has no `Kind` set. This is
because, we only persist the default Kind when the Binding
object is created/updated. The reconciler also sets the defaults
by calling `SetDefaults` but that change is not persisted to etcd.

In the future, we might want to consider updating the object from the
reconciler itself if any default values were set.

Fixes: tektoncd#459

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
@dibyom dibyom changed the title wip:Resolve TriggerBindings with no Kind set in sink Resolve TriggerBindings with no Kind set in sink Feb 28, 2020
@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 Feb 28, 2020
@akihikokuroda
Copy link
Member

/approve

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.

I tested this out and was able to upgrade from v0.2.1 without any problems 👍
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akihikokuroda, 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 Feb 28, 2020
@tekton-robot tekton-robot merged commit e43f662 into tektoncd:master Feb 28, 2020
@dibyom dibyom deleted the binding branch February 28, 2020 20:14
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. cla: yes lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem upgrading to v0.3.0
5 participants