get custom domains from own models instead of AMC db#1239
get custom domains from own models instead of AMC db#1239melvinsoft wants to merge 3 commits intomainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
@melvinsoft thanks a lot for removing AMC db call from the command.
I think we need a new field AlternativeDomain.cert_pending to distinguish between pending certs and ready certs. Such logic shouldn't be needed whenever we use an external service like Treafik, but until then I think this fix is needed.
I've checked the code and I'm not sure if it'll work well or not given that we don't store the cert_pending state anywhere and we may be moving from one unreliable to another unreliable workflow.
Please also add tests for this code.
|
@OmarIthawi Thank you so much for the review. Here is the issue, as you know we have been talking about fixing custom domains and move to something like Traefik, so adding a new field here, also means we need to change logic in Tahoe cert agent and our Ansible playbooks, I don't even know how to start implementing that logic. I understand the tests, but this command is only being used by Tahoe cert agent, the piece we want to deprecate, so I don't think it worth it. In the other hand, I already consumed all my coding buffers for this sprint and this tasks, so I cannot invest more myself in this, what I achieved is something it can be executed with a runbook or a script, and allow an engineer to set up a new custom domains in about 1 hr, with our current conflicting priorities and capacity, we shouldn't fix this further at this stage. |
OmarIthawi
left a comment
OmarIthawi
left a comment
There was a problem hiding this comment.
Thanks again Maxi Sorry for not making my original review clearer.
Required changes: Please add tests now that AMC database isn't accessed.
Suggested change: Add we agreed that we'll not perform this change.cert_pending field
Suggested change: Only return active sites.
| cursor.execute("SELECT custom_domain FROM organizations_microsite WHERE custom_domain != '';") | ||
| rows = cursor.fetchall() | ||
| domains = [{'domains': row} for row in rows] | ||
| altertive_domains = AlternativeDomain.objects.values_list('site__domain', flat=True) |
There was a problem hiding this comment.
Recommended: Can we filter by get_active_sites?
| altertive_domains = AlternativeDomain.objects.values_list('site__domain', flat=True) | |
| altertive_domains = AlternativeDomain.objects.filter( | |
| site__in=get_active_sites(), | |
| ).values_list('site__domain', flat=True) |
As long as the Ansible playbook uses www-data this change should be okay.
- name: List custom domains
become: www-data
shell: ./manage lms custom_domains_list 
|
Checking git merge conflicts against https://github.com/edx/edx-platform.git
|
|
I added a simple test to show how easy it is. It should be expanded with some more test cases to, eg, verify that Omar's recommendation of only including active sites works correctly. |
There was a problem hiding this comment.
Thanks @melvinsoft and @thraxil. This pull request looks great already after adding the tests.
Let's get it merged and I may add the get_active_sites later.
If you're happy with it, please merge it.
4 participants
Change description
Soon we will deprecate AMC and the postgres database won't exist anymore, I realized setting up a custom domains, all the info this commands gets from the AMC database is the same as the alternative domains
Type of change
Related issues
This PRs works together with:
appsembler/configuration#410
Checklists
Development
Security
Code review