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

Pegasus: remove various subdomain sites #22865

Merged
merged 6 commits into from Jun 12, 2018

Conversation

breville
Copy link
Member

@breville breville commented Jun 3, 2018

'sg.code.org' => 'code.org',
'tr.code.org' => 'robincode.org',
'uk.code.org' => 'code.org',
'za.code.org' => 'codeforchange.co.za/',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wjordan Is this the right place to do such redirects?

If so, what's the best way to test that this works?

And is it safe to include the removal of these sites from pegasus in this PR as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't touched or looked at any of this code since 2015 so I can't answer any of your questions confidently, but I'll give my best guesses.

  • I think this is an OK place to do redirects

  • Test manually after merging the attribute changes- this feature was never developed in a way that could be tested in an isolated environment. Note that a redirect for ar.code.org won't also provide a redirect for staging-ar.code.org here, so you'll need to include those if you want to test on staging, otherwise just test once it's pushed to production.

  • Removing the subdomains from CDO.partners will remove them from the CloudFront distribution, which will cause those requests to fail unless you manually update the DNS entries first to send requests directly to the load balancer instead of through the CloudFront distribution. Each partner has the following manual DNS entries:

  • staging-za.code.org: ALIAS staging.code.org.

  • test-za.code.org: ALIAS test.code.org.

  • za.code.org: ALIAS code.org.

  • localhost.za.code.org: ALIAS localhost.code.org.

I'd suggest deleting all but the production one, which should have its alias updated to point to the production-codeorg load balancer.

Beyond that, I can't think of anything else that will definitely break from removing the partner sites in this PR, but I can't guarantee safety beyond whatever our tests currently cover.

Copy link
Contributor

@Erin007 Erin007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach seems reasonable, but this is huge change. I wonder if it makes sense to remove one subdomain first in this way, see if anything crumbles, then remove the others?

breville added a commit that referenced this pull request Jun 7, 2018
This extracts some minimal logic from #22865 to redirect ar.code.org to an external site.

By removing it from the list of partner sites, the subdomain will be removed from the CloudFront distribution.  Before deploying, I will update the DNS entry for ar.code.org to send requests directly to our load balancer instead of CloudFront.  Then the redirect provided via chef to varnish will operate successfully after this change is deployed to production.
@caleybrock
Copy link
Contributor

CC @davidsbailey - great news for pegasus / content pipeline!

@davidsbailey
Copy link
Member

screen shot 2018-06-11 at 9 45 31 am

excellent!!!

@breville
Copy link
Member Author

@Erin007 That was a good call, and exactly what I've been doing in #22976. Thanks.

Conflicts:
	cookbooks/cdo-varnish/attributes/default.rb
	deployment.rb
@breville
Copy link
Member Author

Folks, now that #22865 is working on production (see https://ar.code.org), this PR is ready for final review.

assert_equal 'https://uk.code.org/about', url_converter.replace_origin('https://uk.code.org/about')
assert_equal 'https://ro.code.org', url_converter.replace_origin('https://ro.code.org')
assert_equal 'https://ro.code.org/about', url_converter.replace_origin('https://ro.code.org/about')
assert_equal 'https://advocacy.code.org', url_converter.replace_origin('https://advocacy.code.org')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change seems unrelated to this PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that we were losing tests on uk.code.org and ro.code.org I wanted to add a couple additional tests on a subdomain that remained, advocacy.code.org.

Copy link
Contributor

@caleybrock caleybrock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One inline question, but otherwise looks good!

@breville breville merged commit 259d008 into staging Jun 12, 2018
@breville breville deleted the remove-various-subdomain-sites branch June 12, 2018 18:15
@dju90 dju90 mentioned this pull request Mar 6, 2021
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants