posthog icon indicating copy to clipboard operation
posthog copied to clipboard

fix(cdp): Change StreamName -> StreamARN in Kinesis template

Open bigdefect opened this issue 1 year ago • 2 comments

Problem

https://docs.aws.amazon.com/kinesis/latest/APIReference/API_PutRecord.html#API_PutRecord_RequestSyntax

The inputs ask for an ARN but the code populates the API's Name parameter. Best I can tell, this was mislabeled even in the first iteration of this template, and right now it fails in an obvious way in the function test logs.

You could use the StreamName, but Kinesis recommends the ARN. I think it's also nice to see the account in which the stream was created.

When invoking this API, you must use either the StreamARN or the StreamName parameter, or both. It is recommended that you use the StreamARN input parameter when you invoke this API.

Changes

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Does this work well for both Cloud and self-hosted?

I believe this would not change the behavior across those environments.

How did you test this code?

When trying to set up my first instance of the Kinesis template, I noticed it fails in an obvious way, in that the ARN does not succeed the API's regex for StreamName. Changing the input to just the stream name does allow the function to at least pass that step.

I haven't explicitly tested this change, but it's at least in line with the API documentation.

bigdefect avatar Oct 04 '24 20:10 bigdefect

Hey @bigdefect , I had to re-implement your PR since it required changes to a test, and I can't make changes to contributor PRs :)

However, reading

I haven't explicitly tested this change, but it's at least in line with the API documentation.

Before merging, can you click "show function source code", and make the modification to the function, and try it out? If it works, I'll be happy to merge the new PR, or free feel to copy over the test changes into this branch.

mariusandra avatar Oct 14 '24 13:10 mariusandra

@mariusandra Thanks! I didn't realize I could edit the function code, so I hadn't tried it then. Sadly I've torn down all of our posthog resources after some evaluation, so I can't easily perform a test. Best I can say is that, given those parameters are 1:1 to the API parameters, I'm confident, but I get the desire to explicitly test.

If you can't spare the time to test it, you can always leave it StreamName and change the description of the input field.

bigdefect avatar Oct 15 '24 17:10 bigdefect

This was closed with https://github.com/PostHog/posthog/pull/25711

mariusandra avatar Oct 21 '24 19:10 mariusandra