ARC-1429: Return 404 if Jira has been renamed. #1268
Conversation
…1429-sentry-error-405-s-from-renamed-jira-hosts
… github.com:atlassian/github-for-jira into ARC-1429-sentry-error-405-s-from-renamed-jira-hosts
src/jira/client/axios.ts
Outdated
| try { | ||
| await axios.get("/status", { baseURL }); | ||
| } catch (e) { | ||
| if (e.response.status === 503) { | ||
| logger.info(`503 from Jira: Jira instance '${baseURL}' has been deactivated, is suspended or does not exist. Returning 404 to our application.`); | ||
| error.response.status = 404; | ||
| } | ||
| if (e.response.status === 302) { |
There was a problem hiding this comment.
nit: use else/if instead of if
There was a problem hiding this comment.
If I use else if I will have to define another else at the end, but there's nothing else I want to do there. Do I just do else { return; } ?
There was a problem hiding this comment.
else if doesn't force to have else at the end, so skip else if not required
There was a problem hiding this comment.
Ooo nice I didn't know that, I thought else was required. Thanks Harminder, just updated :)
| @@ -147,13 +147,16 @@ const instrumentRequest = (response) => { | |||
| const instrumentFailedRequest = (baseURL: string, logger: Logger) => { | |||
| return async (error: AxiosError) => { | |||
| instrumentRequest(error?.response); | |||
| if (error.response?.status === 503) { | |||
| if (error.response?.status === 503 || error.response?.status === 405) { | |||
There was a problem hiding this comment.
Ideally this should be feature-flagged...
There was a problem hiding this comment.
Because we are changing the behaviour?
There was a problem hiding this comment.
I'm considering it fine because of the comment below.
| try { | ||
| await axios.get("/status", { baseURL }); | ||
| } catch (e) { | ||
| if (e.response.status === 503) { | ||
| logger.info(`503 from Jira: Jira instance '${baseURL}' has been deactivated, is suspended or does not exist. Returning 404 to our application.`); | ||
| error.response.status = 404; | ||
| } else if (e.response.status === 302) { |
There was a problem hiding this comment.
but probably not a big deal cause this is error handling
There was a problem hiding this comment.
Yeah I think it's ok because I'm just adding more logging to an error that is already being thrown. If these conditions aren't met, we will reject it anyways on line 163.
atrigueiro
deleted the
ARC-1429-sentry-error-405-s-from-renamed-jira-hosts
branch
This is closely related to #1193
Because we don't listen to callbacks from Jira when an instance is renamed we end up making requests to the old and new Jira hosts.
We obviously want to treat this better in the future and disable suspended/renamed installations, but for now, this is a better way for us to track this.
I'll also update the Vacuuming page to include this scenario.