feature: add support for GCP pub sub emulator sink #794
Conversation
There was a problem hiding this comment.
Great work, @girishramnani! Impressed with the thoroughness here.
See comments in-line. If you can, sign off your PR with make signoff - see instructions in CONTRIBUTING.md
Please test to make sure it works with an emulator GCP. Also, make sure both creates and edits work (and that during edits, you can switch emulator on/off)
Also, might make sense to add a test to test/sequin/gcp_pubsub_test.exs? Ask Claude Sonnet to help :)
| bind:checked={form.sink.use_emulator} | ||
| onCheckedChange={toggleEmulator} | ||
| /> | ||
| <Label for="use-emulator" class="align-text-top">Use Emulator</Label> |
There was a problem hiding this comment.
| <Label for="use-emulator" class="align-text-top">Use Emulator</Label> | |
| <Label for="use-emulator" class="align-text-top">Use emulator</Label> |
There was a problem hiding this comment.
Can you add an info popover here that describes what the emulator is? You can see examples of info popovers elsewhere in the codebase. The copy:
The GCP Pub/Sub emulator lets you run Pub/Sub locally for dev and testing purposes. [Learn more](https://cloud.google.com/pubsub/docs/emulator)
Learn more should be a target=_blank link
| @@ -122,6 +122,8 @@ export type GcpPubsubConsumer = BaseConsumer & { | |||
| project_id: string; | |||
| topic_id: string; | |||
| connection_id: string; | |||
| use_emulator: false; | |||
| connection_url: string; | |||
There was a problem hiding this comment.
| connection_url: string; | |
| emulator_base_url: string; |
There was a problem hiding this comment.
Please make this change everywhere below
| <p class="text-destructive text-sm">{errors.sink.emulator_url}</p> | ||
| {/if} | ||
| </div> | ||
| {/if} |
There was a problem hiding this comment.
Should we hide credentials field below when the emulator is enabled? If it's unused, we should hide it.
There was a problem hiding this comment.
Yeah good idea
| {"authorization", "Bearer #{token}"}, | ||
| {"content-type", "application/json"} | ||
| ] | ||
| pubsub_base_url = get_pubsub_base_url(client) |
There was a problem hiding this comment.
I think you can do this in-line:
| pubsub_base_url = get_pubsub_base_url(client) | |
| base_url = Map.get(client.req_opts, :base_url, @pubsub_base_url) |
| defp get_pubsub_headers(%Client{} = client) do | ||
| base_url = Map.get(client.req_opts, :base_url) | ||
|
|
||
| # if there is no base url then we are using the production gcp | ||
| if is_nil(base_url) do |
There was a problem hiding this comment.
Right now, this uses the presence of base_url in req_opts to determine if we should add auth headers.
How about instead we just only add auth headers if Client.credentials is not nil?
There was a problem hiding this comment.
Below, in authenticated_request, you can do:
headers = [{"content-type", "application/json"}] |> maybe_put_auth_headers(client)There was a problem hiding this comment.
Someone might just keep the credentials.
Imagine a user flow.
They added the pub sub details and that didn’t work - so they selected the emulator but never removed to credentials.
There was a problem hiding this comment.
@girishramnani That's a good point. In that case, I think we should nilify the credentials - perhaps in the conditional_ changeset function you added?
That way, there aren't credentials "hidden" in the sink. Would be surprising later to edit the sink, turn off the emulator, and see that there were credentials in there the whole time!
Basically, just trying to have as few things know about the emulator/base URL as possible
There was a problem hiding this comment.
Yeah that would be best
Co-authored-by: Anthony Accomazzo <[email protected]>
Co-authored-by: Anthony Accomazzo <[email protected]>
Co-authored-by: Anthony Accomazzo <[email protected]>
Closes #791
use emulatorswitch for sinkconnection urlinput when selected theuse emulatorswitchOverall providing support for the GCP PubSub Emulator in the sink.