Skip to content
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

Merged
merged 4 commits into from
Dec 5, 2021

Conversation

calebcartwright
Copy link
Member

@calebcartwright calebcartwright commented Oct 27, 2019

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

@calebcartwright calebcartwright added core Server, BaseService, GitHub auth on-hold Deferred in favor of another approach, blocked on preceding efforts, stale, or abandoned labels Oct 27, 2019
@shields-ci
Copy link

shields-ci commented Oct 27, 2019

Messages
📖 ✨ Thanks for your contribution to Shields, @calebcartwright!

Generated by 🚫 dangerJS against 6f2f361

@joelgallant
Copy link

@calebcartwright what was the status of this?

@paulmelnikow
Copy link
Member

Should we give this change another shot?

@calebcartwright
Copy link
Member Author

@paulmelnikow, sure whenever is feasible for you from a deployment perspective

(and apologies @joelgallant I must have missed the notification from your message 😬)

@paulmelnikow
Copy link
Member

Let's merge it and I'll deploy it tomorrow.

paulmelnikow
paulmelnikow previously approved these changes Apr 29, 2020
@paulmelnikow
Copy link
Member

Given we're moving forward with Heroku, I think we should go ahead and merge this.

@calebcartwright
Copy link
Member Author

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?

@paulmelnikow
Copy link
Member

Yea, we can keep an eye on the performance. And importantly, if we need more capacity, we can add it!

@calebcartwright
Copy link
Member Author

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

@chris48s
Copy link
Member

@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..

paulmelnikow
paulmelnikow previously approved these changes May 22, 2020
@calebcartwright calebcartwright changed the title restore cors header injection from #4171 restore cors header injection from #4171, run [github npm travis] May 23, 2020
@chris48s chris48s changed the title restore cors header injection from #4171, run [github npm travis] restore cors header injection from #4171, run [npm travis] May 23, 2020
chris48s
chris48s previously approved these changes May 23, 2020
paulmelnikow
paulmelnikow previously approved these changes May 27, 2020
@calebcartwright
Copy link
Member Author

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

@paulmelnikow paulmelnikow removed the on-hold Deferred in favor of another approach, blocked on preceding efforts, stale, or abandoned label Jul 8, 2020
@calebcartwright
Copy link
Member Author

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

@calebcartwright
Copy link
Member Author

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.

@calebcartwright calebcartwright merged commit 38c1e2d into master Dec 5, 2021
@calebcartwright calebcartwright deleted the restore-cors-injection-4171 branch December 5, 2021 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Server, BaseService, GitHub auth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blocked by CORS policy
6 participants