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: pass correct content type for publish csr - OKTA-288634 #157
fix: pass correct content type for publish csr - OKTA-288634 #157
Conversation
src/generated-client.js
Outdated
@@ -224,6 +227,7 @@ class GeneratedApiClient { | |||
]; | |||
|
|||
const request = this.http.postJson(url, { | |||
headers: { 'Content-Type': 'application/x-x509-ca-cert' }, |
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.
application/x-x509-ca-cert
is a string, http.postJson
is going to try JSON stringify that and fail?
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.
looks like the http.json will only try to set
{
'Content-Type': 'application/json',
'Accept': 'application/json'
}
as default header, but override it by using provided headers from input. No JSON.stringify logic on body.
But yes, this change is confusing... I'll make changes.
0f1a379
to
957dcc8
Compare
const request = this.http.post( | ||
url, | ||
{ | ||
headers: { |
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.
headers changes for operation without responseModel
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.
Our docs do show these headers for these operations, so this is technically more correct. Have you tried running the full IT test suite with these changes? Since we weren't sending these headers before, I do want to sanity check that everything is still passing.
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.
All tests passed.
FYI, we need to skip tests for signingCSR due to key limitations per org, it will be re-enabled when FF available. I tested them locally by targeting to my dev org, and all green.
const request = this.http.post( | ||
url, | ||
{ | ||
headers: { |
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.
Please review headers change here.
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 looks correct, was there something in particular that you are concerned about?
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.
nope, since it is hidden in generated code, just want you to be aware of this change.
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.
Please see comment in review about running full IT test suite, approving under the assumption that all those tests are passing.
const request = this.http.post( | ||
url, | ||
{ | ||
headers: { |
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 looks correct, was there something in particular that you are concerned about?
const request = this.http.post( | ||
url, | ||
{ | ||
headers: { |
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.
Our docs do show these headers for these operations, so this is technically more correct. Have you tried running the full IT test suite with these changes? Since we weren't sending these headers before, I do want to sanity check that everything is still passing.
{{/if}} | ||
{{/if}} | ||
{{/if}} | ||
const request = this.http.{{getHttpMethod this}}( |
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.
👍 for moving logic out of the template and into code
957dcc8
to
029fedb
Compare
All tests passed. |
* fix: pass correct content type for publish csr - OKTA-288634
* updates integration tests for new openapi * updates licensing banners * updated generated code * build from even updateder openapi * chore: sync with openapi master * fix: reference model data generation (#149) * chore: sync with openapi master * chore: sync with openapi master * chore: sync with openapi master * add integration tests for idp okta 288649 (#150) * test: add idp-crud it * test: it for idp-user operations * test: refactor idp-crub to use model methods * refactor: remove commented code in idp-user * test: it for idp-lifecycle * test: add list social tokens test for idp-user * test: it for idp-credential * fix: remove only and counter * test: add more assertions for key test suite * test: add it for user type api - OKTA-288654 (#153) * Integration tests for linked object api okta 288651 (#152) * test: add it for group api - OKTA-288652 (#154) * test: add integration tests for sms template - OKTA-288648 (#145) * test: add integration tests for sms template - OKTA-288648 * test: add comment and change test file name to map endpoints url * test: fix async clean & move mock data to mocks folder * chore(travis): run build before test * Revert "chore(travis): run build before test" This reverts commit b256985. * integration tests for policy api okta 288650 (#151) * test: it for policy-crud * test: it for policy-lifecycle * test: it for policy-rule * test: add it for user api - OKTA-288653 (#155) * test: it for user-linked-object * test: it for applink * test: it for user-client * test: it for user-idp * test: add and merge user lifecycle tests into user-lifecyle * test: add missing lifecycle tests into user-lifecyle * fix: idp test name * test: add it for user grant * fix: add assert to check if returned linked idp match with created idp * fix: support x-okta-multi-operation - OKTA-289242 (#156) * chore: sync with latest openapi master * chore: sync with openapi master * chore: sync with openapi master * fix: pass correct content type for publish csr - OKTA-288634 (#157) * fix: pass correct content type for publish csr - OKTA-288634 * test: add integration tests for event hook apis - OKTA-288645 (#164) * test: add integration tests for event hook apis - OKTA-288645 * test: add doc link in comment * test: add ITs for auth server - OKTA-288644 (#163) * test: add it for auth server * fix: template generator to pass body to collection request * test: enable skipped factor test - OKTA-288643 (#162) * test: skip csr test suites (#159) * test: add it for application - OKTA-288642 (#161) * chore: sync with latest openapi master * test: add integration tests for feature api - OKTA-288647 (#166) * test: add integration tests for inline hook api - OKTA-288646 (#165) * chore: sync with latest openapi * test: add it for admin role - OKTA-288641 (#158) * test: add it for admin role - OKTA-288641 * test: update test with openapi updates * chore: sync with latest openapi master * chore: drop support for node8 - OKTA-257574 (#167) * doc: add changelog - OKTA-305277 (#169) * doc: add changelog * doc: changelog fix * doc: update readme with changed method name * test: re-enable skipped key store tests * test: refactor mock data with faker (#170) * chore: sync with openapi master * chore: sync with openapi * chore: rename case sensitive files * test: fix link object fake names * test: use different user profiles on client-list-users test * chore: remove generated files before build (#172) * chore: update openapi version to latest in npm * doc: add migration guide Co-authored-by: Shuo Wu <wushuo2010@gmail.com>
Resolves: OKTA-288634