-
Notifications
You must be signed in to change notification settings - Fork 0
fix: ensure x-app-uuid is present when using token #37
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
Conversation
The AppLink control plane requires this header to be present as part of validations, so lets ensure it is present. Additionally, lets set a valid User-Agent header to indicate this is from the SDK. Ref: W-18723611
62b5ac3
to
bac92e5
Compare
src/utils/request.ts
Outdated
const response = await fetch(url, opts); | ||
const defaultOpts = { | ||
headers: { | ||
"User-Agent": `heroku-applink-node-sdk/1.0`, |
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.
Nit: we should be able to use fs
to load package.json
and get the version from there.
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.
This bends my brain a bit - just using something like:
const pjson = fs.readFileSync(path.join(__dirname, "..", "package.json"), "utf8");
const pkg = JSON.parse(pjson);
const defaultOpts = {
headers: {
"User-Agent": `heroku-applink-node-sdk/${pkg.version}`,
},
};
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.
Ah, I forget __dirname isn't available in modules and have to do some hackery.
@@ -5,14 +5,23 @@ | |||
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause | |||
*/ | |||
|
|||
type AppUuid = string; |
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.
Curious, why use a branded type?
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.
This is a Go habit of declaring "this thing really is a UUID instead of literally any string"
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.
Thank you for catching and fixing 🙇
@brahyt-sf the test failures here look similar to the ones we are addressing in #36. I'm thinking since they are only failing on ubuntu we should go ahead and merge then try to resolve in the pr you have open. Wdyt? |
f933ee8
to
651e498
Compare
@ekozilforce Yeah we can try that, I will address this failure in that PR. I also wanted to address some other things in that PR, its mistakenly Open and not in Draft. |
The AppLink control plane requires this header to be present as part of validations, so lets ensure it is present.
Additionally, lets set a valid User-Agent header to indicate this is from the SDK.
Ref: W-18723611