feat: add Pub/Sub REST API#65
Conversation
hectorvent
left a comment
There was a problem hiding this comment.
Thanks for this. Really nice addition, and I like that it reuses the existing PubSubService and gates cleanly through the service registry, so no extra config is needed, which is the right call.
One thing I'd want sorted before merging: the two PATCH handlers don't match GCP's REST wire format. Canonical Pub/Sub wraps the resource and carries updateMask in the body, not as a query param:
{ "topic": { "labels": {...}, "messageRetentionDuration": "604800s" },
"updateMask": "labels,messageRetentionDuration" }Refs: pubsub.proto UpdateTopic/UpdateTopicRequest and the REST topics.patch docs. Right now a canonical REST client would send updateMask in the body with fields nested under topic, so the controller reads them as null and ends up wiping the fields. Create, publish, pull, and ack are all fine. It's just the two updates.
Could we make the handlers accept both shapes? Read updateMask from the query param or the body, and pull fields from body.get("topic")/body.get("subscription") when present, falling back to flat. That keeps Terraform working and also covers SDK REST clients.
Small related note: the integration test drives the same flat shape the controller expects, so it can't catch this divergence. Worth a quick SDK or provider based check if easy, otherwise no big deal.
Everything else looks good to me.
Summary
Validation