Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: refactor sdk-snippets to use keys array in webhook response #1096

Merged
merged 5 commits into from
Oct 24, 2024

Conversation

AndriiAndreiev
Copy link
Collaborator

@AndriiAndreiev AndriiAndreiev commented Oct 8, 2024

🚥 Resolves #1085

Made a changes to webhook snippets to use keys array in response

@gratcliff gratcliff added the bug Something isn't working label Oct 8, 2024
@gratcliff gratcliff self-requested a review October 8, 2024 16:10
@AndriiAndreiev AndriiAndreiev marked this pull request as draft October 9, 2024 16:23
@AndriiAndreiev
Copy link
Collaborator Author

AndriiAndreiev commented Oct 9, 2024

@gratcliff @azinder1
can you take a look at the new changes? is that a correct format of "keys" array?
image

@AndriiAndreiev
Copy link
Collaborator Author

AndriiAndreiev commented Oct 9, 2024

do we also need to provide such fields in keys array:

  • type ("http"/"oauth")
  • scheme ("basic"/"bearer")
    or something like this?

@azinder1
Copy link

azinder1 commented Oct 9, 2024

@gratcliff @azinder1 can you take a look at the new changes? is that a correct format of "keys" array? image

I just went through each version, and it looks good! For bearer security typeswe should be able to pass
keys: {'bearer': '123'} (https://github.com/readmeio/metrics-sdks/blob/main/packages/sdk-snippets/src/targets/node/express/webhooks/client.ts#L94-L97). Is there a reason why we're shifting the code so much too? I believe that the current version should mostly work but we'd just need to use the array of object via the keys array and we'd use apiKey for API keys.

It would be useful to see if we can write some tests around the new code since it does appear to run as expected but it's hard to tell without any sort of testing that shows what's returned.

@AndriiAndreiev AndriiAndreiev changed the title Feat/snippets keys array feat: refactor sdk-snippets to use keys array in webhook response Oct 10, 2024
@AndriiAndreiev AndriiAndreiev marked this pull request as ready for review October 10, 2024 15:53
@AndriiAndreiev AndriiAndreiev marked this pull request as draft October 10, 2024 15:55
@AndriiAndreiev
Copy link
Collaborator Author

here's example of the new "keys" array format
image

@azinder1
Copy link

Great 👍 that's exactly how it should look.

@AndriiAndreiev AndriiAndreiev marked this pull request as ready for review October 11, 2024 08:22
@AndriiAndreiev AndriiAndreiev merged commit d285c74 into main Oct 24, 2024
46 checks passed
@AndriiAndreiev AndriiAndreiev deleted the feat/snippets-keys-array branch October 24, 2024 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Update code snippets for all languages to use keys array
3 participants