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

Feature flag to make readthedocs theme default on MkDocs docs #4802

Merged
merged 2 commits into from Oct 24, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 25 additions & 0 deletions readthedocs/doc_builder/backends/mkdocs.py
Expand Up @@ -16,6 +16,7 @@

from readthedocs.doc_builder.base import BaseBuilder
from readthedocs.doc_builder.exceptions import BuildEnvironmentError
from readthedocs.projects.models import Feature

log = logging.getLogger(__name__)

Expand Down Expand Up @@ -50,6 +51,23 @@ def __init__(self, *args, **kwargs):
self.root_path = self.version.project.checkout_path(self.version.slug)
self.yaml_file = self.get_yaml_config()

# README: historically, the default theme was ``readthedocs`` but in
# https://github.com/rtfd/readthedocs.org/pull/4556 we change it to
# ``mkdocs`` to maintain the same behavior in Read the Docs than
# building locally. Although, we can't apply the this into the Corporate
Copy link
Member

Choose a reason for hiding this comment

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

the this -> this

# site. To keep the same default theme there, we created a Feature flag
# for these project that were building with MkDocs in the Corporate
# site.
if self.project.has_feature(Feature.MKDOCS_THEME_RTD):
self.DEFAULT_THEME_NAME = 'readthedocs'
log.warning(
'Project using readthedocs theme as default for MkDocs: slug=%s',
self.project.slug,
)
else:
self.DEFAULT_THEME_NAME = 'mkdocs'


def get_yaml_config(self):
"""Find the ``mkdocs.yml`` file in the project root."""
mkdoc_path = self.config.mkdocs.configuration
Expand Down Expand Up @@ -130,6 +148,13 @@ def append_conf(self, **__):
# This supports using RTD's privacy improvements around analytics
user_config['google_analytics'] = None

# README: make MkDocs to use ``readthedocs`` theme as default if the
# user didn't specify a specific theme manually
if self.project.has_feature(Feature.MKDOCS_THEME_RTD):
if 'theme' not in user_config:
# mkdocs<0.17 syntax
user_config['theme'] = self.DEFAULT_THEME_NAME
Copy link
Contributor

Choose a reason for hiding this comment

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

The thing I think @davidfischer got caught up on here is that this might not work/break when a user pins mkdocs>0.17. Can you confirm the user has a way to self-select out of our feature flag?


# Write the modified mkdocs configuration
yaml.safe_dump(
user_config,
Expand Down
35 changes: 35 additions & 0 deletions readthedocs/projects/migrations/0028_feature-flag-mkdocs-theme.py
@@ -0,0 +1,35 @@
# -*- coding: utf-8 -*-
# Generated by Django 1.11.16 on 2018-10-24 07:43
from __future__ import unicode_literals

from django.db import migrations


FEATURE_ID = 'mkdocs_theme_rtd'


def forward_add_feature(apps, schema_editor):
Feature = apps.get_model('projects', 'Feature')
Feature.objects.create(
feature_id=FEATURE_ID,
# Not using ``default_true=True`` because we will do this manually in
# the database from the Corporate site only, since this is not required
# in the Community site
# default_true=True,
)


def reverse_add_feature(apps, schema_editor):
Feature = apps.get_model('projects', 'Feature')
Feature.objects.filter(feature_id=FEATURE_ID).delete()


class Migration(migrations.Migration):

dependencies = [
('projects', '0027_remove_json_with_html_feature'),
]

operations = [
migrations.RunPython(forward_add_feature, reverse_add_feature),
]
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're not doing anything like setting a default with the migration, then we don't actually need it for feature flags to work. I guess we'll have to manually add a feature flag on the commercial side?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, the migration in this case only creates the Feature flag object in the database.

If we don't want that, the migration is useless and I can remove it. As you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this is not required for feature flags. It's only needed if we are setting a time-based default.

2 changes: 2 additions & 0 deletions readthedocs/projects/models.py
Expand Up @@ -1061,6 +1061,7 @@ def add_features(sender, **kwargs):
SKIP_SUBMODULES = 'skip_submodules'
DONT_OVERWRITE_SPHINX_CONTEXT = 'dont_overwrite_sphinx_context'
ALLOW_V2_CONFIG_FILE = 'allow_v2_config_file'
MKDOCS_THEME_RTD = 'mkdocs_theme_rtd'

FEATURES = (
(USE_SPHINX_LATEST, _('Use latest version of Sphinx')),
Expand All @@ -1072,6 +1073,7 @@ def add_features(sender, **kwargs):
'Do not overwrite context vars in conf.py with Read the Docs context',)),
(ALLOW_V2_CONFIG_FILE, _(
'Allow to use the v2 of the configuration file')),
(MKDOCS_THEME_RTD, _('Use Read the Docs theme for MkDocs as default theme')),
)

projects = models.ManyToManyField(
Expand Down
63 changes: 62 additions & 1 deletion readthedocs/rtd_tests/tests/test_doc_builder.py
Expand Up @@ -20,7 +20,7 @@
from readthedocs.doc_builder.backends.sphinx import BaseSphinx
from readthedocs.doc_builder.python_environments import Virtualenv
from readthedocs.projects.exceptions import ProjectConfigurationError
from readthedocs.projects.models import Project
from readthedocs.projects.models import Feature, Project


class SphinxBuilderTest(TestCase):
Expand Down Expand Up @@ -224,6 +224,67 @@ def test_get_theme_name(self, checkout_path):
}
self.assertEqual(builder.get_theme_name(config), 'mydir')

@patch('readthedocs.doc_builder.base.BaseBuilder.run')
@patch('readthedocs.projects.models.Project.checkout_path')
def test_get_theme_name_with_feature_flag(self, checkout_path, run):
tmpdir = tempfile.mkdtemp()
checkout_path.return_value = tmpdir
Feature.objects.get(
feature_id=Feature.MKDOCS_THEME_RTD,
).projects.add(self.project)

python_env = Virtualenv(
version=self.version,
build_env=self.build_env,
config=None,
)
builder = MkdocsHTML(
build_env=self.build_env,
python_env=python_env,
)
self.assertEqual(builder.get_theme_name({}), 'readthedocs')
with patch('readthedocs.doc_builder.backends.mkdocs.yaml') as mock_yaml:
with patch('readthedocs.doc_builder.backends.mkdocs.MkdocsHTML.load_yaml_config') as mock_load_yaml_config:
mock_load_yaml_config.return_value = {'site_name': self.project.name}
builder.append_conf()

mock_yaml.safe_dump.assert_called_once_with(
{
'site_name': mock.ANY,
'docs_dir': mock.ANY,
'extra_javascript': mock.ANY,
'extra_css': mock.ANY,
'google_analytics': mock.ANY,
'theme': 'readthedocs',
},
mock.ANY,
)
mock_yaml.reset_mock()

config = {
'theme': 'customtheme',
}
self.assertEqual(builder.get_theme_name(config), 'customtheme')
with patch('readthedocs.doc_builder.backends.mkdocs.MkdocsHTML.load_yaml_config') as mock_load_yaml_config:
mock_load_yaml_config.return_value = {
'site_name': self.project.name,
'theme': 'customtheme',
}
builder.append_conf()

mock_yaml.safe_dump.assert_called_once_with(
{
'site_name': mock.ANY,
'docs_dir': mock.ANY,
'extra_javascript': mock.ANY,
'extra_css': mock.ANY,
'google_analytics': mock.ANY,
'theme': 'customtheme',
},
mock.ANY,
)


@patch('readthedocs.doc_builder.base.BaseBuilder.run')
@patch('readthedocs.projects.models.Project.checkout_path')
def test_append_conf_create_yaml(self, checkout_path, run):
Expand Down