Add feature flipping models to Projects, plus example feature flip #3194

agjohnson · 2017-10-25T22:02:20Z

This add per-project feature flags that can be changed via the django admin. Features can be added via signal, though this is only required to list the feature in the admin dashboard forms, it isn't required to use the feature flags in code.

Because of some initial problems with modeling, this also updated the pattern we're using for Project/Version model instances in the build servers. These models make the relationship to the underlying data more explicit, that is, we're not using what looks to be a Project/Version, but rather a representation of the API return. I think this pattern does a better job of making this clear, though I think ultimately we want something completely divorced from the Project/Version models.

An example feature flip that we need is included for the Sphinx version.

Also, use a new pattern for API instance objects

agjohnson · 2017-10-27T15:47:49Z

Uh, on second though, I hit an issue where having a many2many relationship to projects makes more sense. I think i'll redo the relationship.

I wanted to add a feature for defaulting to true before $start_date, so we'll want some extra fields on the feature and only 1 feature object for each feature added. I'm mostly certain the middle ground is not requiring data migrations, but for features like deprecating old code, we'll want to create a migration that immediately sets up the feature as true before $start_date.

This allows for features on deprecated code, where we can say the feature is true historically.

agjohnson · 2017-10-27T23:42:12Z

Okay, this should be set. I've reworked the relationship, added feature filtering based on the feature add date, and added some new tests.

ericholscher

LGTM, other than a few small nits.

ericholscher · 2017-10-30T20:43:45Z

readthedocs/builds/models.py

+        self.project = APIProject(**kwargs.pop('project', {}))
+        # This was replicated from readthedocs.projects.utils.make_api_project.
+        # I'm not certain why these need to be deleted
+        for key in ['resource_uri', 'absolute_url', 'downloads']:


Because they are fake objects that aren't on the model, but created by the API.

Could also invent the logic and delete anything that isn't a field on the project.

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.

ericholscher · 2017-10-30T20:45:04Z

readthedocs/doc_builder/python_environments.py

-            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')


If it's the latest, shouldn't it just do sphinx without a version ID?

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.

Maybe do semvar and <2?

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 setuptools PR.

ericholscher · 2017-10-30T20:46:26Z

readthedocs/projects/admin.py

+        if feature.projects.count() > 10:
+            projects += [
+                '... and {0} more'.format(feature.projects.count() - 10)
+            ]


Surprised this doesn't get done automatically. Does it output the whole list?

This view probably isn't useful in this view anyhow. I'll just simplify and make it a count.

ericholscher · 2017-10-30T20:47:41Z

readthedocs/projects/migrations/0020_add-api-project-proxy.py

+                'proxy': True,
+            },
+            bases=('projects.project',),
+        ),


Curious that Django makes a migration for a proxy model. I assume it doesn't do anything to the DB?

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.

Yeah, i was surprised by this too at first, i didn't realize this when doing the integrations proxy models.

ericholscher · 2017-10-30T20:50:52Z

readthedocs/projects/models.py

+
+    FEATURES = (
+        (USE_SPHINX_LATEST, _('Use latest version of Sphinx')),
+    )


Any reason we store these on the model? We usually have a constants file, so it seems odd to break that convention.

This is the prescribed method of defining constants:
https://docs.djangoproject.com/en/1.11/ref/models/fields/#django.db.models.Field.choices

ericholscher · 2017-10-30T20:51:30Z

readthedocs/projects/models.py

+    )
+    # 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(


Feature.feature is a weird name here. It should probably be id or similar.

feature_id would be fine

ericholscher · 2017-10-30T20:52:07Z

readthedocs/projects/querysets.py

+    def for_project(self, project):
+        return self.filter(
+            Q(projects=project) |
+            Q(default_true=True, add_date__gt=project.pub_date)


ericholscher · 2017-10-30T23:44:20Z

👍

agjohnson added the PR: work in progress Pull request is not ready for full review label Oct 25, 2017

Add feature flipping models to Projects, plus example feature flip

59cccbf

Also, use a new pattern for API instance objects

agjohnson force-pushed the project-features branch from a7b4595 to 59cccbf Compare October 27, 2017 01:30

agjohnson added 3 commits October 26, 2017 23:14

Refactoring the make_api_* calls into the API models

d251bbd

Add migrations for proxy models

21bab41

Add some admin features

954f632

agjohnson removed the PR: work in progress Pull request is not ready for full review label Oct 27, 2017

agjohnson requested a review from ericholscher October 27, 2017 05:36

This was referenced Oct 27, 2017

Add feature flip for setuptools latest version #3200

Merged

Add feature flip for always upgrading user defined packages on pip install #3201

Merged

agjohnson added the PR: work in progress Pull request is not ready for full review label Oct 27, 2017

agjohnson added 2 commits October 27, 2017 13:09

Rework feature relationship, add default true value based on date

e6d2bdc

This allows for features on deprecated code, where we can say the feature is true historically.

Fix some issues, more tests

adbe72a

agjohnson removed the PR: work in progress Pull request is not ready for full review label Oct 27, 2017

agjohnson mentioned this pull request Oct 28, 2017

Add feature flip for build endpoints #3205

Merged

ericholscher approved these changes Oct 30, 2017

View reviewed changes

agjohnson added 2 commits October 30, 2017 16:21

Rename Feature.feature -> Feature.feature_id, other cleanup

dbdf59f

Use semver instead

5aa4696

agjohnson merged commit b65e2f3 into master Oct 31, 2017

agjohnson deleted the project-features branch October 31, 2017 05:27

Uh oh!

Add feature flipping models to Projects, plus example feature flip #3194

Add feature flipping models to Projects, plus example feature flip #3194

Uh oh!

Conversation

agjohnson commented Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agjohnson commented Oct 27, 2017

Uh oh!

agjohnson commented Oct 27, 2017

Uh oh!

ericholscher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericholscher commented Oct 30, 2017

Uh oh!

Uh oh!

agjohnson commented Oct 25, 2017 •

edited

Loading