-
-
Notifications
You must be signed in to change notification settings - Fork 221
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
fix(#9799): add user-agent header to outbound #9818
base: master
Are you sure you want to change the base?
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.
Such a quick turnaround! nice!
I left some minor suggestions inline.
shared-libs/outbound/src/outbound.js
Outdated
sendOptions.headers = { | ||
Authorization: value | ||
}; | ||
sendOptions.headers.Authorization = value; |
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.
http headers must be lowercase in http2: https://www.rfc-editor.org/rfc/rfc7540#section-8.1.2
I know we're not using http2 everywhere (yet), http1 headers are case-insensitive so might as well respect the stricter standard.
sendOptions.headers.Authorization = value; | |
sendOptions.headers.authorization = value; |
hello @dianabarsan @witash should we upgrade to this branch in the UI in the meantime as the work here still goes on ? |
@dianabarsan this is ready for re-reivew, moved deploy-info to shared-libs/environment so it could be used in both the API and other shared-libs, and refactored outbound.js to use aysnc/await instead of explicit Promises. @gitcliff I think it should be ok to upgrade your instance to this branch temporarily |
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.
Very nice. Just some minor comments here.
adds user-agent header to outbound
User-Agent headers should include versions, which means a little extra stuff; it tries to get the CHT version in the same way as api/src/services/deploy-info.js, but that code is not directly available from shared-libs, so there is some small duplication.
Added getVersion() to shared-libs/settings. It doesn't quite fit there, but I don't think its worth it to make another separate shared-lib
We could also just omit the version if it adds too much mess, but I think these small additions are ok.
closes #9799