Remove gpubsub in sources #1761
Conversation
google-cla
bot
added
the
cla: yes
zhongduo
changed the title
|
/assign @nachocano |
|
The following jobs failed:
Automatically retrying due to test flakiness... |
|
The following jobs failed:
Automatically retrying due to test flakiness... |
| // If there is an error, the projectID will be empty. The reconciler will retry | ||
| // to get the projectID during reconciliation. | ||
| projectID, err := utils.ProjectID(os.Getenv(utils.ProjectIDEnvKey), metadataClient.NewDefaultMetadataClient()) | ||
| if err != nil { | ||
| logging.FromContext(ctx).Error("Failed to get project ID", zap.Error(err)) | ||
| } | ||
| // Attempt to create a pubsub client for all worker threads to use. If this | ||
| // fails, pass a nil value to the Reconciler. They will attempt to | ||
| // create a client on reconcile. | ||
| client, err := pubsub.NewDefaultPubsubClient(ctx, projectID) | ||
| if err != nil { | ||
| logging.FromContext(ctx).Error("Failed to create controller-wide Pub/Sub client", zap.Error(err)) | ||
| } | ||
|
|
||
| if client != nil { | ||
| go func() { | ||
| <-ctx.Done() | ||
| client.Close() | ||
| }() | ||
| } | ||
|
|
There was a problem hiding this comment.
this will not apply to sources as we can specify the project in the CO spec.
There was a problem hiding this comment.
the idea of having it in the spec was to support cross-project stuff.
There was a problem hiding this comment.
either way, if you are doing this in the broker, if you cannot create a client here, might be better to fail?
There was a problem hiding this comment.
After our conversation yesterday, I actually removed this too. See the latest code.
And yes, you are right about the failing the broker. There are some legacy code, I have discussed this with others and opened an issue to revisit whether we should fail it.
zhongduo
force-pushed
the
remove-gpubsub
branch
from
7372f7f
to
34d834c
Compare
|
/test pull-google-knative-gcp-unit-tests |
Topic and PullSubscription reconcilers still use the gpubsub client, making their tests different from the Broker and Trigger tests which use pstest. With this, error injection is done by setting server option to the pubsub testing server. The reconcilers will use google-cloud-go pubsub directly.
zhongduo
force-pushed
the
remove-gpubsub
branch
from
0d22aab
to
e44aadf
Compare
|
The following is the coverage report on the affected files.
|
|
/test pull-google-knative-gcp-integration-tests |
|
/assign @capri-xiyue |
|
/assign @Harwayne |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: capri-xiyue, zhongduo 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 |
| "k8s.io/client-go/tools/record" | ||
| ) | ||
|
|
||
| // CreateFn is a factory function to create a Pub/Sub client. It is copied from gclient/pubsub |
There was a problem hiding this comment.
What is wrong with the dependency? Does it cause a cycle?
There was a problem hiding this comment.
I think Jimmy is trying to remove the usage of gpubsub in source code and UT(which was written by ourselves) and change gpubsub to pubsub and pbtest(written by pubsub team)
Fixes #1739
Proposed Changes
google-cloud-gofor error injection: feat(pubsub/pstest): Add reactor options to pstest server googleapis/google-cloud-go#2916pubsub.Clientis used to replace gpubsub version, replace gpubsub error injection with server error injection.Release Note
Docs