get_sites: configurable backend to filter active sites #321
Conversation
|
@johnbaldwin this isn't much ready for merge but should have everything I got in mind. Please review it and let me know what do you think especially regarding the inline notes in the code. |
| # Would be really Really REALLY great to be able to filter out dead sites | ||
|
|
||
| sites = Site.objects.all() | ||
| sites = get_sites() |
| # Would be really Really REALLY great to be able to filter out dead sites | ||
|
|
||
| sites = Site.objects.all() | ||
| sites = get_sites() |
|
|
||
| :return list of Site (QuerySet) | ||
| """ | ||
| sites_backend_path = settings.ENV_TOKENS['FIGURES'].get('SITES_BACKEND') |
There was a problem hiding this comment.
Just to note: Confirming it is ok to assume settings.ENV_TOKENS['FIGURES'] will always exist.
Figures sets a default empty dict for settings.ENV_TOKENS['FIGURES']. See here: https://github.com/appsembler/figures/blob/0.4.dev7/figures/settings/lms_production.py#L80
| sites_backend_path = settings.ENV_TOKENS['FIGURES'].get('SITES_BACKEND') | ||
| if sites_backend_path: | ||
| sites_backend = import_from_path(sites_backend_path) | ||
| sites = sites_backend() |
There was a problem hiding this comment.
Was first thinking, why not just do sites = import_from_path(sites_backend_path)() But this is probably clearer
| @@ -378,8 +376,8 @@ def populate_monthly_metrics_for_site(site_id): | |||
| @shared_task | |||
| def run_figures_monthly_metrics(): | |||
| """ | |||
| TODO: only run for active sites. Requires knowing which sites we can skip | |||
| Populate monthly metrics for all sites. | |||
There was a problem hiding this comment.
Technically, this is is only all sites when a filtered sites backend is not used
| @@ -899,8 +899,10 @@ class SiteViewSet(StaffUserOnDefaultSiteAuthMixin, viewsets.ReadOnlyModelViewSet | |||
| Access is restricted to global (Django instance) staff | |||
| """ | |||
| model = Site | |||
| queryset = Site.objects.all() | |||
There was a problem hiding this comment.
You should probably just be able to set queryset = figures.sites.get_sites() and not need the get_queryset method. I'd need to look through the DRF code to see if there is any significant difference
There was a problem hiding this comment.
If get_sites returns an already evaluated list (and memory cached) there could be a problem of having the list of sites stale.
If it did have a stale sites, it means the only way to refresh the list is to restart the workers.
While this issue may not happen, it would be a rabbit hole to discover it.
Therefore I'm erring on the safe side of using a method since it's out of the control of Figures.
| @@ -220,3 +221,30 @@ def test_first_last_days_for_month(): | |||
| assert last_day.year == year | |||
| assert first_day.day == 1 | |||
| assert last_day.day == 29 | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
@OmarIthawi Nice simple set of tests here, I like them!
|
@johnbaldwin where to document the Figures settings? If there's no configuration doc, where should be created? |
OmarIthawi
force-pushed
the
omar/sites-backend
branch
from
4eec20c to
d2c3c0e
Compare
RED-1721. This will help to make Figures use the
openedx.core.djangoapps.appsembler.sites.utils.get_active_sitesvia configurations to make the pipeline run only for active sites.This should help as both stopgap and long term solution for the increasing time of the pipeline.
TODO