Revert removal of template.Name field and handle Deprecation of template.Name field #912

savitaashture · 2021-01-19T11:08:39Z

Issue: slack discussion
RootCause: https://tektoncd.slack.com/archives/CL3T51NRF/p1611039325020300?thread_ts=1610982177.013500&cid=CL3T51NRF

Changes

Reverted back changes done as part of this PR and handled Deprecation for template.Name field
Will remove template.Name field may be in the next release v0.12.0

We may need to do patch release to handle this issue

Signed-off-by: Savita Ashture [email protected]

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

Deprecate template.Name in favor of template.Ref

savitaashture · 2021-01-19T11:09:45Z

/cc @dibyom @khrm @MarcelMue

MarcelMue · 2021-01-19T11:14:00Z

pkg/apis/triggers/v1alpha1/trigger_types.go

-	Spec       *TriggerTemplateSpec `json:"spec,omitempty"`
+	// Deprecated: Use Ref instead
+	// To be removed in a later release #911
+	DeprecatedName string               `json:"name,omitempty"`


Not blocking just wondering:
Why does the omitempty fix the problem described in slack?

Because omitempty ensures that when this struct is store, it stores nothing related to DeprecatedName field but Ref using defaults. Previously, default wasn't working properly because it stored "".

What I mean is: We have removed the name field completely already, creating a new CR should therefore not have the field anymore. So even if it was stored with "" we should fix anything going wrong in that case.

Readding the field here kind of implies that users first need to upgrade to this specific version before updating to the next minor.

I think we can recommend the user to directly upgrade to v0.11.0 from v0.10.2

So then it would make sense to put this change into v0.10.3 instead of v0.11.1, right?

yeah doing a 0.10.3 is also an option. Though that would mean users would first have to move to v0.10.3 and then to v0.11.0/v0.11.1

khrm · 2021-01-19T11:39:56Z

pkg/apis/triggers/v1alpha1/event_listener_defaults.go

@@ -30,6 +30,7 @@ func (el *EventListener) SetDefaults(ctx context.Context) {
 		for i := range el.Spec.Triggers {
 			triggerSpecBindingArray(el.Spec.Triggers[i].Bindings).
 				defaultBindings()
+			templateNameToRef(el.Spec.Triggers[i].Template)


Call this function only if there's Template isn't nil. I think that's why test is failing.

Yes its failing because of that

Updating

tekton-robot · 2021-01-19T12:00:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: khrm

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:

~~OWNERS~~ [khrm]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tekton-robot · 2021-01-19T12:01:56Z

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_defaults.go	100.0%	95.8%	-4.2
pkg/apis/triggers/v1alpha1/trigger_validation.go	94.0%	94.4%	0.3

tekton-robot · 2021-01-19T12:15:56Z

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_validation.go	94.0%	94.4%	0.3

dibyom · 2021-01-19T15:35:17Z

Summarizing the issue here:

We had logic in triggers_default.go to change template.Name to template.Ref. But because the Name field did not have a omitempty tag, the resource still contained a Name: "" field. This makes it impossible to upgrade running trigger resources from v0.10.2 to v0.11.0

(another candidate for #619)

dibyom · 2021-01-19T15:38:15Z

EasyCLA Expected — Waiting for status to be reported

🙄

dibyom · 2021-01-19T15:40:21Z

/lgtm

MarcelMue · 2021-01-19T15:54:49Z

Fine with me - I think it's the easiest way forward for users.

/lgtm

dibyom · 2021-01-19T15:59:54Z

Gonna try to close/re-open this PR for trigger the CLA again

…ate.Name field

tekton-robot · 2021-01-19T16:02:27Z

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_validation.go	94.0%	94.4%	0.3

savitaashture · 2021-01-19T16:03:17Z

Gonna try to close/re-open this PR for trigger the CLA again

I have done force push

tekton-robot · 2021-01-19T16:03:25Z

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_validation.go	94.0%	94.4%	0.3

dibyom · 2021-01-19T16:11:01Z

/lgtm

Add links to versioned docs for v0.11.0

a849c00

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jan 19, 2021

tekton-robot requested review from iancoffey and vtereso January 19, 2021 11:08

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 19, 2021

tekton-robot requested review from dibyom, khrm and MarcelMue January 19, 2021 11:09

MarcelMue reviewed Jan 19, 2021

View reviewed changes

khrm approved these changes Jan 19, 2021 •

edited

Loading

View reviewed changes

tekton-robot assigned khrm Jan 19, 2021

tekton-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 19, 2021

khrm reviewed Jan 19, 2021

View reviewed changes

savitaashture force-pushed the dep-name branch from 5af3ed0 to 515f9c1 Compare January 19, 2021 12:00

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 19, 2021

savitaashture force-pushed the dep-name branch from 515f9c1 to 9c5fec1 Compare January 19, 2021 12:14

afrittoli mentioned this pull request Jan 19, 2021

Adapt tekton resources for triggers v11 tektoncd/plumbing#682

Merged

2 tasks

tekton-robot assigned dibyom Jan 19, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 19, 2021

tekton-robot assigned MarcelMue Jan 19, 2021

dibyom closed this Jan 19, 2021

dibyom reopened this Jan 19, 2021

Revert removal of template.Name field and handle Deprecation of templ…

34391b1

…ate.Name field

savitaashture force-pushed the dep-name branch from 9c5fec1 to 34391b1 Compare January 19, 2021 16:02

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 19, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 19, 2021

tekton-robot merged commit 530737b into tektoncd:release-v0.11.x Jan 19, 2021

dibyom mentioned this pull request Jan 19, 2021

Revert removal of template.Name field and handle Deprecation of template.Name field #913

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert removal of template.Name field and handle Deprecation of template.Name field #912

Revert removal of template.Name field and handle Deprecation of template.Name field #912

savitaashture commented Jan 19, 2021 •

edited

Loading

savitaashture commented Jan 19, 2021

MarcelMue Jan 19, 2021

khrm Jan 19, 2021

MarcelMue Jan 19, 2021

khrm Jan 19, 2021

MarcelMue Jan 19, 2021

dibyom Jan 19, 2021

khrm Jan 19, 2021

savitaashture Jan 19, 2021

tekton-robot commented Jan 19, 2021

tekton-robot commented Jan 19, 2021

tekton-robot commented Jan 19, 2021

dibyom commented Jan 19, 2021 •

edited

Loading

dibyom commented Jan 19, 2021

dibyom commented Jan 19, 2021

MarcelMue commented Jan 19, 2021

dibyom commented Jan 19, 2021

tekton-robot commented Jan 19, 2021

savitaashture commented Jan 19, 2021

tekton-robot commented Jan 19, 2021

dibyom commented Jan 19, 2021

Revert removal of template.Name field and handle Deprecation of template.Name field #912

Revert removal of template.Name field and handle Deprecation of template.Name field #912

Conversation

savitaashture commented Jan 19, 2021 • edited Loading

Changes

Submitter Checklist

Release Notes

savitaashture commented Jan 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Jan 19, 2021

tekton-robot commented Jan 19, 2021

tekton-robot commented Jan 19, 2021

dibyom commented Jan 19, 2021 • edited Loading

dibyom commented Jan 19, 2021

dibyom commented Jan 19, 2021

MarcelMue commented Jan 19, 2021

dibyom commented Jan 19, 2021

tekton-robot commented Jan 19, 2021

savitaashture commented Jan 19, 2021

tekton-robot commented Jan 19, 2021

dibyom commented Jan 19, 2021

savitaashture commented Jan 19, 2021 •

edited

Loading

dibyom commented Jan 19, 2021 •

edited

Loading