feat: Support Pub/Sub push notifications format (adapt to CloudEvent) #238
Conversation
|
Note: tested with both the PubSub emulator and the production service |
src/Google.Cloud.Functions.Framework/GcfEvents/GcfConverters.cs
Outdated
Show resolved
Hide resolved
jskeet
force-pushed
the
reformat-pubsub
branch
from
c4dc501 to
8daf30b
Compare
src/Google.Cloud.Functions.Framework/GcfEvents/GcfConverters.cs
Outdated
Show resolved
Hide resolved
jskeet
force-pushed
the
reformat-pubsub
branch
from
8daf30b to
29367d9
Compare
| then the Functions Framework will infer the topic name from the path | ||
| of the HTTP request. If the topic name cannot be inferred | ||
| automatically, a topic name of | ||
| `projects/unknown-project!/topics/unknown-topic!` will be used in |
There was a problem hiding this comment.
It seems like we aren't consistent about what we use here across languages.
- In Node.js we just drop it and make the CE subject
//pubsub.googleapis.com/ - In Ruby we set it to
//pubsub.googleapis.com/UNKNOWN_PUBSUB_TOPIC
Also, I think it is might be possible to always extract the project name from the subscription in the payload.
There was a problem hiding this comment.
It seems like we aren't consistent about what we use here across languages.
Mmm. The reason I chose this is to keep structure that will work in the absence of "real" data, but make it obvious that it's not real data. That may or may not be a good choice. Let's discuss this more before actually releasing.
Also, I think it is might be possible to always extract the project name from the subscription in the payload.
That assumes that the subscription is in the same project as the topic, which may not be the case. (I think cross-project PubSub subscriptions are a thing, with appropriate IAM. I could be wrong.) For the emulator that doesn't matter, but for the genuine PubSub push notifications, I think it's better to give either "obviously wrong" data or "user-specified data" (the URL path) rather than "looks plausible but may be wrong" data.
We should definitely talk about that part more. I'm going to merge this as-is, but file a new issue so that we don't forget about it before the next release.
Fixes #234