Reconcilers delete old Topics and PullSubscriptions #1066
Conversation
This is being done because the intevents version will soon need the same functions and will be given the non-prefixed names (as they will continue to exist past the next release).
|
This is ready for review. |
|
/retest |
1 similar comment
|
/retest |
| @@ -175,15 +189,101 @@ func (psb *PubSubBase) ReconcilePullSubscription(ctx context.Context, pubsubable | |||
| } | |||
| } | |||
|
|
|||
| // TODO Compare the actual PullSubscription we found with the desired one and update as needed. | |||
There was a problem hiding this comment.
Nope :)
Bad merge with your change that just added this.
| return ps, nil | ||
| } | ||
|
|
||
| func (psb *PubSubBase) deleteOldPubSubTopicCO(_ context.Context, pubsubable duck.PubSubable, t *inteventsv1alpha1.Topic) pkgreconciler.Event { |
There was a problem hiding this comment.
nit: need the CO at the end? same for the pullSubscription deletion
There was a problem hiding this comment.
something that has been useful for me in this deprecation stuff has been to add TODOs saying remove after 0.x is cut, or something like that... not strong opinion, but might be easier for others to know what to remove
| "knative.dev/pkg/apis" | ||
| "knative.dev/pkg/kmeta" | ||
|
|
||
| duckv1alpha1 "github.com/google/knative-gcp/pkg/apis/duck/v1alpha1" | ||
| ) | ||
|
|
||
| type Identifiable interface { | ||
| // runtime.Object can be removed once the old Topic and PullSubscription are removed. | ||
| runtime.Object |
There was a problem hiding this comment.
The event recorder requires a runtime.Object. In the deleteOldPubSub*CO methods, I write an event if the old CO is not owned by the pubsubable.
|
/unhold Comments addressed. |
|
The following is the coverage report on the affected files.
|
|
/lgtm
…On Thu, May 21, 2020 at 12:18 PM Knative Metrics Robot < ***@***.***> wrote:
The following is the coverage report on the affected files.
Say /test pull-google-knative-gcp-go-coverage to re-run this coverage
report
File Old Coverage New Coverage Delta
pkg/reconciler/intevents/reconciler.go
<https://storage.cloud.google.com/knative-prow/pr-logs/pull/google_knative-gcp/1066/pull-google-knative-gcp-go-coverage/1263548678933581826/artifacts/line-cov.html#file0> Do
not exist 79.9%
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#1066 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABD65DDU76ZCK23NPJUP6KLRSV5ABANCNFSM4NAGTHMQ>
.
|
Fixes #905.
Proposed Changes
TopicandPullSubscriptioninpubsub.cloud.google.com.NoDelete, so that the Topic is not deleted in GCP.Servicename, so we will never have both beReady=trueat the same time.Ready=true.Release Note
Docs