-
Notifications
You must be signed in to change notification settings - Fork 2
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
FTDCS-151 - Use aws-sdk official package to sign requests to AWS #93
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think one or two smaller things but otherwise this is looking good 👍 do we want to pursue a review from Storytelling as code owners as well? Also unsure if Alex is part-way through a review
lib/helpers/signed-aws-es-fetch.js
Outdated
async function signedFetch(url, opts, creds) { | ||
creds = creds || {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: could this be a default param?
async function signedFetch(url, opts, creds) { | |
creds = creds || {}; | |
async function signedFetch(url, opts, creds = {}) { |
This isn't working when we pull it into next-article and it hasn't been released yet. We don't understand this work enough to debug it and get it working and it's blocking our n-logger work from being merged and released. We'd like to revert everything. As it appeared in two different PRs, I decided to revert it commit by commit. The PRs are: - #86 - #93 The commit hashes I reverted in order are: - 826ca12 - b848fa4 - 5f6b1f5 - ffef3f8 - ce7e591 - 8af8c24 - 2c49ac9 - 0e5ec8f - 2ed8545 - 698302b - 6fd9cd5 - 6fd9cd5 - 2f229b8 - 48f79ce I'll be installing this branch in next-article before merging so that I can be sure that I didn't miss anything and that it's safe to install the latest version of n-es-client in our apps.
No description provided.