google / knative-gcp Goto Github PK

cc @mattmoor

And here, esp. the bit about metadata?

Originally posted by @vaikas-google in https://github.com/GoogleCloudPlatform/cloud-run-events/diffs

Consider turning this into an issue. Something about status of sinks might not get reflected, there might be issues with this on error conditions.

Consider turning this into an issue. Something about status of sinks might not get reflected, there might be issues with this on error conditions.

Originally posted by @vaikas-google in https://github.com/_render_node/MDE3OlB1bGxSZXF1ZXN0UmV2aWV3MjY4MDUyNjQy/pull_request_reviews/more_threads

Occasionally I see that Storage creates two GCS notifications. Then, when the Storage CO is deleted, only the more recent notifications is deleted. This caused me to hit the limit of 10 notifications on a bucket.

$ gsutil notification list gs://eventing-demo
# Nothing

$ kn importers create storage desjani --parameters bucket=eventing-demo
# Finishes when the importer is ready.

$ gsutil notification list gs://eventing-demo
projects/_/buckets/eventing-demo/notificationConfigs/67
        Cloud Pub/Sub topic: projects/harwayne2/topics/storage-e711b0c4-f647-4723-be61-99ce59801287
        Custom attributes:
                knative-gcp: google.storage

projects/_/buckets/eventing-demo/notificationConfigs/68
        Cloud Pub/Sub topic: projects/harwayne2/topics/storage-b7cbfa63-a7ba-4f8f-8306-efc574daa7e4
        Custom attributes:
                knative-gcp: google.storage

$ ./kn importers generic delete storage desjani  
# Finishes successfully.

$ gsutil notification list gs://eventing-demo
projects/_/buckets/eventing-demo/notificationConfigs/67
        Cloud Pub/Sub topic: projects/harwayne2/topics/storage-e711b0c4-f647-4723-be61-99ce59801287
        Custom attributes:
                knative-gcp: google.storage

You will want to use the kmeta.ChildName method here to avoid overly long names

Originally posted by @n3wscott in #187

We could use now - LastTransitionTime, but maybe not if LastTransitionTime is updated when the message changes.

According to @n3wscott we actually want to timestamp the point when observedGeneration != spec.generation and use that as the subtrahend.

Originally posted by @grantr in #35 (comment)

Per: #10 (comment)

Create a GCS importer with just the "finalize" event type.
Then edit it to add "metadataupdate". Webhook accepts it and the object is ready.
However, gsutil notification list doesn't show the new type. (also old ones stay forever, but I believe there is another bug tracking it).

I haven't tested for other fields.

Expected behavior:
Either make the field immutable, or reconcile (ideal) it.

I want to use the same function in both places.

#26 (comment)

This is where I found some confusion, coupled with the lines below where we actually create it. They seem very similar to just create. Or put another way, why do you need subCreates and don't just throw everything into subUpdates. I think this is where the comments would help. It was difficult to visually diff what really was different about these two loops for the create case.

Originally posted by @vaikas-google in #26

This should be a method on the type.

Originally posted by @grantr in #35

ref: #10 (comment)

Does this need the +optional tag?

Originally posted by @grantr in #34

This code seems unnecessary. updateFinalizers is only called if we changed the finalizers during reconcile. IIUC the only thing this is protecting against is if something else changes the object while we reconciled, but we can let the patch fail and try again, right?

Originally posted by @grantr in #35 (comment)

We need an E2E for the storage source/importer. It will have to do the proper cluster setup (e.g., creating secrets, a bucket, etc.), then upload a file, and see that a notification is received on the sink.

There is a working example with PullSubscription, where we might be able to reuse some of the code. Need to sync with @n3wscott

wonder if we should explicitly say which permissions this Service account should have?

Originally posted by @vaikas-google in #10

In PR knative/eventing#1849, we added ClusterRole to get, list and watch "pullsubscriptions" and "storages" in config/200-source-resolvers-clusterrole.yaml.
This is a temporary solution. Such ClusterRole should eventually be moved to config directories in https://github.com/google/knative-gcp.

can we bring up the test coverage for the components on a followup PR...
/lgtm
/approve

Originally posted by @vaikas-google in #124 (comment)

Use Semantic.DeepEqual in topic reconciler

Originally posted by @grantr in #35 (comment)

Publisher should be a ksvc.

Originally posted by @n3wscott in #35

Use Semantic.DeepEqual here, not cmp.Diff. cmp.Diff is meant for tests. https://github.com/google/go-cmp

Originally posted by @grantr in #35

It'd be nice if envconfig had a method for creating this from the structured type :-/

Originally posted by @mattmoor in #51 (comment)

the cloudEventOverrides should propagate down into the PullSubscription, not added to the customAttributes

Originally posted by @n3wscott in #169

Re: https://github.com/google/knative-gcp/pull/203/files#r316266232

I guess there is something to do with performance or whatever so serving has been making this test instead of doing the asserts like above: https://github.com/google/knative-gcp/blob/master/pkg/apis/pubsub/v1alpha1/implements_test.go#L26

it lets you test round trips in json form so that is useful.

Originally posted by @n3wscott in #203

Default Storage secrets.

https://github.com/google/knative-gcp/pull/153/files#r313621340

When using Event Display (which uses Cloud SDK), both Storage and Scehduler examples show errors in the output:
For example:
☁️ cloudevents.Event
Validation: valid
Context Attributes,
specversion: 0.3
type: google.storage.object.finalize
source: //storage.googleapis.com/buckets/vaikasvaikas-notification-test
subject: testfilehere
id: 710717795564381
time: 2019-08-27T16:35:03.742Z
schemaurl: https://raw.githubusercontent.com/google/knative-gcp/master/schemas/storage/schema.json
datacontenttype: application/json
Extensions,
error: unexpected end of JSON input

Problem
A request made to the Channel should generate traces that link the request to the subscribers. It should either use an existing Trace if present, or create a new Trace if one isn't.

The trace from the Channel to any subscribers MUST be annotated with the subscription's name.

Persona:
System Integrator

Exit Criteria
Each of the following generate a trace that includes both the incoming request and the request to any subscribers. The trace from the Channel to any subscribers MUST be annotated with the subscription's name:

• A request sent to an Channel without any trace information.
• A request sent to an Channel with trace information in the HTTP headers (B3 format).
• A request sent to an Channel with trace information in the Cloud Event's traceparent attribute, but not in the HTTP headers.

Note that this is very similar to knative/eventing#1757

followed the instructions but job fails with

{"level":"info","ts":1567617015.9008741,"logger":"fallback-logger","caller":"scheduler/job.go:217","msg":"Failed to create Job \"projects/fooo/locations/us-central1/jobs/XXX\": rpc error: code = PermissionDenied desc = Cloud Scheduler API has not been used in project XXX before or it is disabled. Enable it by visiting https://console.cloud.google.com/apis/api/cloudscheduler.googleapis.com/overview?project=XXX then retry. If you enabled this API recently, wait a few minutes for the action to propagate to our systems and retry.","action":"create","project":"fooo","jobName":"projects/fooo/locations/us-central1/jobs/XXX"}

Typo exisiting -> existing https://github.com/GoogleCloudPlatform/cloud-run-events/pull/35/files#diff-1d699b9cd7b3d4b5ad8802c07777a902R324 #35 (comment)

Add a comment saying "This method is generic for future compatibility with an SDK or some such" https://github.com/GoogleCloudPlatform/cloud-run-events/pull/35/files#diff-1d699b9cd7b3d4b5ad8802c07777a902R312 https://github.com/GoogleCloudPlatform/cloud-run-events/pull/35/files#r301248760

Re: #10 (comment)

Root cause: presubmit script is not executable.

The current ServiceAccount name for the controller is controller. This works well while in the cloud-run-events namespace. But we saw in knative/eventing that people wanted to install the controllers and webhooks into user defined namespaces, at which point the name controller is likely to collide.

I recommend cloud-run-events-controller.

Typo.

Originally posted by @grantr in https://github.com/GoogleCloudPlatform/cloud-run-events/diffs/23

This leaves the finalizer hanging around -- should we give up on the finalizer at some point (for example, if we don't have permissions to delete the subscription, or if it is already deleted)?

I seem to recall that the GitHub source had issues where it would stick around forever if (for example) you created a source but it hadn't had a chance to register the webhook yet.

Originally posted by @evankanderson in https://github.com/GoogleCloudPlatform/cloud-run-events/diffs

Doesn't OpenAPI schema support enums - for policy?

Originally posted by @akashrv in #34

cc @akashrv

bit of a nit here. This assumes you have actually downloaded the files (or cloned the git repo). It would be nice if we could have a 'curl' way to get these files so that we don't raise the bar unnecessarily for just kicking the tires.

Originally posted by @vaikas-google in https://github.com/_render_node/MDExOlB1bGxSZXF1ZXN0Mjg2Nzk0NTgz/pull_requests/unread_timeline

This is getting long enough that it should probably be a struct.

Originally posted by @Harwayne in #51 (comment)

This key.json is what I wanted to be documented earlier in the Topic type's Secret ref field. How does the user know to use this key otherwise?

Originally posted by @grantr in #35

Can you add a test using an invalid propagationPolicy value?

Originally posted by @grantr in #34

Why is this PROJECT_ID and not PROJECT?

Originally posted by @grantr in https://github.com/GoogleCloudPlatform/cloud-run-events/pull/35/files/b43fb10e9247b0db8e1abd323dc4814609315d1b

I assume that this is because CloudEvents attributes with Map or Any type are a mess?

cloudevents/spec#467 (if accepted) might help.

Originally posted by @evankanderson in https://github.com/GoogleCloudPlatform/cloud-run-events/diffs

read the condition from PullSubscription, and add a message if it's not ready.

Originally posted by @n3wscott in https://github.com/google/knative-gcp/pull/178/files#r314812446

Is there a reason why we can't use a well defined name and not have to list them using label selectors. I think there might be a race here. We had this same discussion with Channels and decided to use predictable naming and that also has been what serving has been doing.

Originally posted by @vaikas-google in #24

Stub tests need to be filled in: https://github.com/GoogleCloudPlatform/cloud-run-events/pull/66/files#diff-e4aeeba1df8e80967ec05b8535f5d297

Depend on serving and eventing master instead of specific revisions.

Originally posted by @grantr in #35 (comment)

The Webhook and Controller should use distinct ServiceAccounts, as they need different permissions. This also matches the knative/eventing service accounts documentation.