-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Add feature flipping models to Projects, plus example feature flip #3194
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
Changes from 6 commits
59cccbf
d251bbd
21bab41
954f632
e6d2bdc
adbe72a
dbdf59f
5aa4696
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
# -*- coding: utf-8 -*- | ||
# Generated by Django 1.9.12 on 2017-10-27 00:17 | ||
from __future__ import unicode_literals | ||
|
||
from django.db import migrations | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
('builds', '0003_add-cold-storage'), | ||
] | ||
|
||
operations = [ | ||
migrations.CreateModel( | ||
name='APIVersion', | ||
fields=[ | ||
], | ||
options={ | ||
'proxy': True, | ||
}, | ||
bases=('builds.version',), | ||
), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
from readthedocs.doc_builder.config import ConfigWrapper | ||
from readthedocs.doc_builder.loader import get_builder_class | ||
from readthedocs.projects.constants import LOG_TEMPLATE | ||
from readthedocs.projects.models import Feature | ||
|
||
log = logging.getLogger(__name__) | ||
|
||
|
@@ -128,8 +129,14 @@ def install_core_requirements(self): | |
if self.project.documentation_type == 'mkdocs': | ||
requirements.append('mkdocs==0.15.0') | ||
else: | ||
requirements.extend(['sphinx==1.5.3', 'sphinx-rtd-theme<0.3', | ||
'readthedocs-sphinx-ext<0.6']) | ||
if self.project.has_feature(Feature.USE_SPHINX_LATEST): | ||
requirements.append('sphinx<1.7') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's the latest, shouldn't it just do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was curious what we wanted to do here. I'm fine with leaving it bleeding edge, without a specifier. '1.7' specifier was a stopgap from making the version go too high. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe do semvar and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah yeah, that's a really good idea. hopefully they follow semver as well. i'll do that instead of open ended on this and |
||
else: | ||
requirements.append('sphinx==1.5.6') | ||
requirements.extend([ | ||
'sphinx-rtd-theme<0.3', | ||
'readthedocs-sphinx-ext<0.6' | ||
]) | ||
|
||
cmd = [ | ||
'python', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,9 +13,10 @@ | |
from readthedocs.redirects.models import Redirect | ||
from readthedocs.notifications.views import SendNotificationView | ||
|
||
from .notifications import ResourceUsageNotification | ||
from .models import (Project, ImportedFile, | ||
from .forms import FeatureForm | ||
from .models import (Project, ImportedFile, Feature, | ||
ProjectRelationship, EmailHook, WebHook, Domain) | ||
from .notifications import ResourceUsageNotification | ||
from .tasks import remove_dir | ||
|
||
|
||
|
@@ -180,8 +181,26 @@ class DomainAdmin(admin.ModelAdmin): | |
model = Domain | ||
|
||
|
||
class FeatureAdmin(admin.ModelAdmin): | ||
model = Feature | ||
form = FeatureForm | ||
list_display = ('feature', 'projects_list', 'default_true') | ||
search_fields = ('feature',) | ||
filter_horizontal = ('projects',) | ||
readonly_fields = ('add_date',) | ||
|
||
def projects_list(self, feature): | ||
projects = [p.name for p in feature.projects.all()[0:10]] | ||
if feature.projects.count() > 10: | ||
projects += [ | ||
'... and {0} more'.format(feature.projects.count() - 10) | ||
] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Surprised this doesn't get done automatically. Does it output the whole list? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This view probably isn't useful in this view anyhow. I'll just simplify and make it a count. |
||
return ', '.join(projects) | ||
|
||
|
||
admin.site.register(Project, ProjectAdmin) | ||
admin.site.register(ImportedFile, ImportedFileAdmin) | ||
admin.site.register(Domain, DomainAdmin) | ||
admin.site.register(Feature, FeatureAdmin) | ||
admin.site.register(EmailHook) | ||
admin.site.register(WebHook) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
# -*- coding: utf-8 -*- | ||
# Generated by Django 1.9.12 on 2017-10-27 12:55 | ||
from __future__ import unicode_literals | ||
|
||
from django.db import migrations, models | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
('projects', '0018_fix-translation-model'), | ||
] | ||
|
||
operations = [ | ||
migrations.CreateModel( | ||
name='Feature', | ||
fields=[ | ||
('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), | ||
('feature', models.CharField(max_length=32, unique=True, verbose_name='Feature identifier')), | ||
('add_date', models.DateTimeField(auto_now_add=True, verbose_name='Date feature was added')), | ||
('default_true', models.BooleanField(default=False, verbose_name='Historical default is True')), | ||
], | ||
), | ||
migrations.AddField( | ||
model_name='feature', | ||
name='projects', | ||
field=models.ManyToManyField(blank=True, to='projects.Project'), | ||
), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
# -*- coding: utf-8 -*- | ||
# Generated by Django 1.9.12 on 2017-10-27 12:56 | ||
from __future__ import unicode_literals | ||
|
||
from django.db import migrations, models | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
('projects', '0019_add-features'), | ||
] | ||
|
||
operations = [ | ||
migrations.CreateModel( | ||
name='APIProject', | ||
fields=[ | ||
], | ||
options={ | ||
'proxy': True, | ||
}, | ||
bases=('projects.project',), | ||
), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious that Django makes a migration for a proxy model. I assume it doesn't do anything to the DB? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://stackoverflow.com/questions/37988914/why-does-django-create-migration-files-for-proxy-models seems to say it's so later migrations can reference it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, i was surprised by this too at first, i didn't realize this when doing the integrations proxy models. |
||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,10 +26,10 @@ | |
from readthedocs.projects.querysets import ( | ||
ProjectQuerySet, | ||
RelatedProjectQuerySet, | ||
ChildRelatedProjectQuerySet | ||
ChildRelatedProjectQuerySet, | ||
FeatureQuerySet, | ||
) | ||
from readthedocs.projects.templatetags.projects_tags import sort_version_aware | ||
from readthedocs.projects.utils import make_api_version | ||
from readthedocs.projects.version_handling import determine_stable_version, version_windows | ||
from readthedocs.restapi.client import api | ||
from readthedocs.vcs_support.backends import backend_cls | ||
|
@@ -639,9 +639,10 @@ def get_latest_build(self, finished=True): | |
return self.builds.filter(**kwargs).first() | ||
|
||
def api_versions(self): | ||
from readthedocs.builds.models import APIVersion | ||
ret = [] | ||
for version_data in api.project(self.pk).active_versions.get()['versions']: | ||
version = make_api_version(version_data) | ||
version = APIVersion(**version_data) | ||
ret.append(version) | ||
return sort_version_aware(ret) | ||
|
||
|
@@ -821,6 +822,55 @@ def add_comment(self, version_slug, page, content_hash, commit, user, text): | |
hash=content_hash, commit=commit) | ||
return node.comments.create(user=user, text=text) | ||
|
||
@property | ||
def features(self): | ||
return Feature.objects.for_project(self) | ||
|
||
def has_feature(self, feature): | ||
"""Does project have existing feature flag | ||
|
||
If the feature has a historical True value before the feature was added, | ||
we consider the project to have the flag. This is used for deprecating a | ||
feature or changing behavior for new projects | ||
""" | ||
return self.features.filter(feature=feature).exists() | ||
|
||
|
||
class APIProject(Project): | ||
|
||
"""Project proxy model for API data deserialization | ||
|
||
This replaces the pattern where API data was deserialized into a mocked | ||
:py:cls:`Project` object. This pattern was confusing, as it was not explicit | ||
as to what form of object you were working with -- API backed or database | ||
backed. | ||
|
||
This model preserves the Project model methods, allowing for overrides on | ||
model field differences. This model pattern will generally only be used on | ||
builder instances, where we are interacting solely with API data. | ||
""" | ||
|
||
features = [] | ||
|
||
class Meta: | ||
proxy = True | ||
|
||
def __init__(self, *args, **kwargs): | ||
self.features = kwargs.pop('features', []) | ||
for key in ['users', 'resource_uri', 'absolute_url', 'downloads', | ||
'main_language_project', 'related_projects']: | ||
try: | ||
del kwargs[key] | ||
except KeyError: | ||
pass | ||
super(APIProject, self).__init__(*args, **kwargs) | ||
|
||
def save(self, *args, **kwargs): | ||
return 0 | ||
|
||
def has_feature(self, feature): | ||
return feature in self.features | ||
|
||
|
||
@python_2_unicode_compatible | ||
class ImportedFile(models.Model): | ||
|
@@ -921,3 +971,63 @@ def delete(self, *args, **kwargs): # pylint: disable=arguments-differ | |
from readthedocs.projects import tasks | ||
broadcast(type='app', task=tasks.symlink_domain, args=[self.project.pk, self.pk, True]) | ||
super(Domain, self).delete(*args, **kwargs) | ||
|
||
|
||
@python_2_unicode_compatible | ||
class Feature(models.Model): | ||
|
||
"""Project feature flags | ||
|
||
Features should generally be added here as choices, however features may | ||
also be added dynamically from a signal in other packages. Features can be | ||
added by external packages with the use of signals:: | ||
|
||
@receiver(pre_init, sender=Feature) | ||
def add_features(sender, **kwargs): | ||
sender.FEATURES += (('blah', 'BLAH'),) | ||
|
||
The FeatureForm will grab the updated list on instantiation. | ||
""" | ||
|
||
# Feature constants - this is not a exhaustive list of features, features | ||
# may be added by other packages | ||
USE_SPHINX_LATEST = 'use_sphinx_latest' | ||
|
||
FEATURES = ( | ||
(USE_SPHINX_LATEST, _('Use latest version of Sphinx')), | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason we store these on the model? We usually have a constants file, so it seems odd to break that convention. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the prescribed method of defining constants: |
||
|
||
projects = models.ManyToManyField( | ||
Project, | ||
blank=True, | ||
) | ||
# Feature is not implemented as a ChoiceField, as we don't want validation | ||
# at the database level on this field. Arbitrary values are allowed here. | ||
feature = models.CharField( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
_('Feature identifier'), | ||
max_length=32, | ||
unique=True, | ||
) | ||
add_date = models.DateTimeField( | ||
_('Date feature was added'), | ||
auto_now_add=True, | ||
) | ||
default_true = models.BooleanField( | ||
_('Historical default is True'), | ||
default=False, | ||
) | ||
|
||
objects = FeatureQuerySet.as_manager() | ||
|
||
def __str__(self): | ||
return "{0} feature".format( | ||
self.get_feature_display(), | ||
) | ||
|
||
def get_feature_display(self): | ||
"""Implement display name field for fake ChoiceField | ||
|
||
Because the field is not a ChoiceField here, we need to manually | ||
implement this behavior. | ||
""" | ||
return dict(self.FEATURES).get(self.feature, self.feature) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because they are fake objects that aren't on the model, but created by the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also invent the logic and delete anything that isn't a field on the project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah gotcha, i recognized these as model attributes without looking. makes sense.
Ultimately, I think we want to some logic that uses the serializer to model these fields modeling for API consumption, and drop Project model proxying altogether. I'm just not sure what the connection to the serializer looks like yet. I think this is probably the best way to reduce duplication of data however.