-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
restore cors header injection from #4171, run [npm travis] #4255
Conversation
|
@calebcartwright what was the status of this? |
Should we give this change another shot? |
@paulmelnikow, sure whenever is feasible for you from a deployment perspective (and apologies @joelgallant I must have missed the notification from your message 😬) |
Let's merge it and I'll deploy it tomorrow. |
Given we're moving forward with Heroku, I think we should go ahead and merge this. |
Are we all set in the new Heroku environment to measure the impact of this change to determine whether it was the cause of the perf issues we saw around the last time it was deployed? |
Yea, we can keep an eye on the performance. And importantly, if we need more capacity, we can add it! |
SGTM. I'll do a rebase and make any updates (guessing eslint related) to get CI passing again, and we can take it from there |
@calebcartwright - are you up for rebasing this to get the tests passing so we can try this out again? Hopefully its quite a small job.. |
f1068d4
to
acbefa9
Compare
77b36e2
to
8725eeb
Compare
haven't forgotten about this. just need to find some time when I can merge and deploy this commit in isolation, and then watch the metrics for a while to see if it causes any spike |
Realize this has been sitting for ages. To recap, this was reverted (along with #4254) because of a potential correlation with a performance event observed in our old OVH environment. I've had it parked for a while for various reasons, including some trial comparison periods we've been running with different http client libs. This should be much easier to deploy in our current Heroku world given how easily we can roll back |
c1aedba
to
cb64466
Compare
Going to merge and deploy this now given that it can be done in isolation (production is currently in sync with the master branch). i'll keep an eye on this today/into the night US time, and check on it again in the morning local time. As a recap this was reverted because of it being a potential cause of performance issues, so if issues are observed today/tomorrow we should be prepared to rollback prod in Heroku. |
Do Not Merge (til after the next deploy)!
Restores the cors header injection updates originally made in #4171 that were temporarily reverted in #4253
Also ref #4227
Closes #3273