-
Notifications
You must be signed in to change notification settings - Fork 4
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
Set result status code when returning a non-200 response #412
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.
Code lgtm! After this is merged and deployed to prod, I think it's going to break the availability test because 500s will start being returned because the unique number in the test is still for prod API gateway (unless you changed it yesterday) even though it's now connected to preprod UAT. Not sure what a good general process is for updating that when deploying, any ideas?
@@ -123,6 +123,11 @@ export const getServerSideProps: GetServerSideProps = async ({ req, locale, quer | |||
// that in our links back out to UIO. | |||
const userArrivedFromUioMobile = query?.from === 'uiom' | |||
|
|||
// If there is an errorCode, set the response statusCode to match. |
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.
// If there is an errorCode, set the response statusCode to match. | |
// If there is an errorCode from the API gateway, set our app's response statusCode to match. |
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.
@kalvinwang I'm not including this suggestion because the error code is set for issues that are not limited to errors from the API gateway. It's any application level error (e.g. env vars missing, catching a thrown error
, etc).
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.
ooh, got it, thanks!
@kalvinwang I think this is an example of how the avalibility tests are brittle, but without an equivalent healthcheck endpoint we can call on the API gateway, I'm not sure there is a better alternative. We have a section of the Foundation doc called Connecting to different API gateway environments. Maybe we should just make that a more robust checklist and include updating the availability tests in that checklist. Thoughts? |
Ticket
Resolves #406
Changes
Context
Right now, if we return an error page, we are still sending a 200 response code. We should be setting the appropriate HTTP response code.
Note: It turns out that writing a unit test for HTTP responses in Next.js is non-trivial, so I broke that out into a separate ticket so we can get the functionality merged now: #411 Here are a few leads:
Testing
yarn dev