V2 of the configuration file #4355

stsewd · 2018-07-10T19:24:57Z

This PR will just add the parser/validation for the v2 schema.

stsewd · 2018-07-11T19:44:28Z

Ok, I have ~147 tests to pass

This ones have a replace in v2

stsewd · 2018-07-16T19:13:50Z

I have re-reviewed this, about the duplicate logic:

validate_formats is different (the default values and accepts a keyword)
validate_build is different
validate_python
- validate_version looks the same
- validate_requirements is different (here an empty string is valid)
- validate_extra_requirements is a little equal (we check that it is used with install: pip)
- validate_system_site_packages is the same
validate_conda is the same but the keys are different

So, I'm not sure if reusing some methods are going to save a lot of code. We have different cases to manage in each version.

stsewd

In the last commits I change the code to respect the defaults from the schema rather than the db, remove some unused keys from v1 that have a replacement in v2, and added a feature flag to deactivate the v2 on production.

stsewd · 2018-07-16T20:05:50Z

readthedocs/config/config.py

+    },
+    'readthedocs/build:stable': {
+        'python': {'supported_versions': [2, 2.7, 3, 3.3, 3.4, 3.5, 3.6]},
+    },
    'readthedocs/build:latest': {
        'python': {'supported_versions': [2, 2.7, 3, 3.3, 3.4, 3.5, 3.6]},


BTW, latest is the same for v1 and v2, right?

For now, yeah.

stsewd · 2018-07-16T20:11:31Z

readthedocs/doc_builder/config.py

@@ -26,16 +27,26 @@ def load_yaml_config(version):

    img_name = project.container_image or DOCKER_IMAGE
    python_version = 3 if project.python_interpreter == 'python3' else 2
+    allow_v2 = project.has_feature(Feature.ALLOW_V2_CONFIG_FILE)


Not sure if this is the best way of using the feature flag here.

Let me know if this is okay, to generate the migration.

I think we support v2 no matter when the project was created, so "ALLOW_V2_CONFIG_FILE" isn't the best name for this. Rather, when projects have this feature flag, we'll assume the user will definitely be using v2 config to configure their project. This allows us to disable the UI elements in the admin for options that can be controlled in config.

I wanted to add this to don't allow users to use the v2 in production yet. Maybe a django setting is the best here? or what name do you suggest for this feature flag?

agjohnson

Looking close. I think there are some changes needed still on the feature flag concept, and I think we probably want a type field, even if it is determined from the use of sphinx vs mkdocs keys.

agjohnson · 2018-07-17T16:48:37Z

readthedocs/config/config.py

+    },
+    'readthedocs/build:stable': {
+        'python': {'supported_versions': [2, 2.7, 3, 3.3, 3.4, 3.5, 3.6]},
+    },
    'readthedocs/build:latest': {
        'python': {'supported_versions': [2, 2.7, 3, 3.3, 3.4, 3.5, 3.6]},


For now, yeah.

agjohnson · 2018-07-17T17:04:02Z

readthedocs/config/config.py

+                    'You can not have the ``sphinx`` and ``mkdocs`` '
+                    'keys at the same time',
+                    code=INVALID_KEYS_COMBINATION
+                )


I get what this particular function is doing, but what i was pointing out is that we aren't using the config file to determine the build type.

Currently, I believe build type is handled here:
https://github.com/rtfd/readthedocs.org/blob/ca7afe6577672e129ccfe63abe33561dc32a6651/readthedocs/doc_builder/python_environments.py#L275

So, currently, it's possible for a sphinx key to exist in the config file, but the user can set mkdocs build type, and we'll use that as the build backend type. I think what we actually want is for the config file to dictate build type. If there is a mkdocs key, we can assume this is a mkdocs build.

agjohnson · 2018-07-17T17:24:05Z

readthedocs/doc_builder/config.py

@@ -26,16 +27,26 @@ def load_yaml_config(version):

    img_name = project.container_image or DOCKER_IMAGE
    python_version = 3 if project.python_interpreter == 'python3' else 2
+    allow_v2 = project.has_feature(Feature.ALLOW_V2_CONFIG_FILE)


I think we support v2 no matter when the project was created, so "ALLOW_V2_CONFIG_FILE" isn't the best name for this. Rather, when projects have this feature flag, we'll assume the user will definitely be using v2 config to configure their project. This allows us to disable the UI elements in the admin for options that can be controlled in config.

agjohnson · 2018-07-17T17:25:37Z

readthedocs/projects/models.py

@@ -1022,6 +1022,7 @@ def add_features(sender, **kwargs):
    SKIP_SUBMODULES = 'skip_submodules'
    BUILD_JSON_ARTIFACTS_WITH_HTML = 'build_json_artifacts_with_html'
    DONT_OVERWRITE_SPHINX_CONTEXT = 'dont_overwrite_sphinx_context'
+    ALLOW_V2_CONFIG_FILE = 'allow_v2_config_file'


Noted above, i think this feature flag name should change

agjohnson · 2018-07-17T17:25:48Z

readthedocs/projects/models.py

@@ -1033,6 +1034,8 @@ def add_features(sender, **kwargs):
            'Build the json artifacts with the html build step')),
        (DONT_OVERWRITE_SPHINX_CONTEXT, _(
            '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')),


Also needs update here

stsewd · 2018-07-18T20:52:08Z

I think this PR is ready for a final review, next step is matching the properties between v1 and v2 (where it makes sense of course). And modify the build process to make use of the new properties, and I think we can implement the new features like submodules here too.

I'll be building some projects to make sure this doesn't break the v1, but it shouldn't, I didn't touch the v1 logic here.

stsewd · 2018-07-18T20:25:58Z

readthedocs/config/config.py

+    valid_sphinx_builders = {
+        'sphinx': 'sphinx',
+        'htmldir': 'sphinx_htmldir',
+        'singlehtml': 'sphinx_singlehtml',


I leave this values as plain htmldir rather than sphinx_htmldir to avoid redundancy in the sphinx key, but we always return the full name from the config object.

should the first builder should be html then, instead of sphinx? I know our mapping has sphinx for the default html builder, but this is not clear, and should probably not be this way anyways. I think {'html': 'sphinx_html'} make more sense for this. Also, from the user perspective, I'd expect to fill in sphinx.builder: html.

Yeah that makes more sense, I wasn't sure what to put there p: Changing

Well, it looks like you followed our existing pattern, it's just that our existing pattern is sort of wrong also. Do we also need to update the logic for deciding on the builder?

We don't need to update the logic here, but yeah when implementing the actual changes in the logic we need to modify that.

About the pattern, I'm not sure if I follow you hehe, do you mean changing the names from sphinx -> html there too? We can create an issue to track that.

👍 that works

agjohnson

I think for consistency the html sphinx keys/value needs to change, but this looks good otherwise. I'm not sure we need to update the design doc, as i think we'll use the schema as a source for generating documentation on the schema, but we could update that if you think it's important. I'm not certain it is though.

agjohnson · 2018-07-19T23:49:02Z

readthedocs/config/config.py

+    valid_sphinx_builders = {
+        'sphinx': 'sphinx',
+        'htmldir': 'sphinx_htmldir',
+        'singlehtml': 'sphinx_singlehtml',


should the first builder should be html then, instead of sphinx? I know our mapping has sphinx for the default html builder, but this is not clear, and should probably not be this way anyways. I think {'html': 'sphinx_html'} make more sense for this. Also, from the user perspective, I'd expect to fill in sphinx.builder: html.

stsewd · 2018-07-20T01:31:46Z

I'm not sure we need to update the design doc

Yeah, I think the same, the design doc was to put an initial path in the project. Having the schema is enough I think. What I was wondering is that we'll need to repeat the schema in the docs, but there aren't many keys anyway.

agjohnson

I think these changes look good to merge! 👍

stsewd added the PR: work in progress Pull request is not ready for full review label Jul 10, 2018

stsewd added 28 commits July 12, 2018 14:14

Add tests for formats

0548e74

Fix tests

d373878

Fix test

1a124ab

Add tests for conda

09c1c5a

Add tests for build

c89544e

Fix test

61a265b

Use new interface

e8a0e60

Add tests for python

e03bc69

Add tests for sphinx

bd18668

Add tests for mkdocs

27c3f5c

Add tests for submodules

64ace4d

Fix tests

3bd4cb4

Add tests for install_project default

e7eb10b

Add tests for formats default

328246c

Add tests for use_system_packages default

2801bf0

Add tests for requirements_file default

56bc31c

Fix tests

ba3f86e

Fix tests

5489dbc

Tests for docker_image default

68033d0

Allow strings and numbers in python.version

0029f43

Test stable image

9722e8b

Fix tests

dc70c07

Add more validators

d044568

Validate formats and conda

f8ed462

Implement build

775f174

Fix tests

2317ce1

More strict validation for lists

3ce8c60

Add python validation

f3a4ea8

Remove unused keys from v1

2e759ce

This ones have a replace in v2

Add feature flag for configuration file (v2)

b4fa44f

stsewd commented Jul 16, 2018

View reviewed changes

agjohnson reviewed Jul 17, 2018

View reviewed changes

stsewd added 7 commits July 17, 2018 20:54

Merge branch 'master' into build-config-v2

893998d

Include sphinx.builder to schema

046693e

Add tests for sphinx.builder key and doctype config property

6dbec8c

Fix tests

05382eb

Check for sphinx.builder

6a99a80

Update test

02c9bda

Pass doctype to defaults

24ed1fb

stsewd commented Jul 18, 2018

View reviewed changes

stsewd removed the PR: work in progress Pull request is not ready for full review label Jul 18, 2018

stsewd requested a review from agjohnson July 18, 2018 20:52

agjohnson reviewed Jul 19, 2018

View reviewed changes

agjohnson added this to the YAML File Completion milestone Jul 20, 2018

Rename sphinx builder option to html

aeefb0c

stsewd requested a review from agjohnson July 23, 2018 16:55

stsewd mentioned this pull request Aug 1, 2018

Match v1 config interface to new one #4456

Merged

agjohnson approved these changes Aug 2, 2018

View reviewed changes

agjohnson merged commit 5be4cd1 into readthedocs:master Aug 2, 2018

stsewd deleted the build-config-v2 branch August 2, 2018 17:11

stsewd restored the build-config-v2 branch August 2, 2018 17:11

This was referenced Aug 2, 2018

Specify version of yaml spec to use #3806

Closed

Improve access to options of the configuration object #4346

Closed

Add submodule configuration readthedocs/readthedocs-build#47

Closed

stsewd deleted the build-config-v2 branch August 16, 2018 04:33

Uh oh!

V2 of the configuration file #4355

V2 of the configuration file #4355

Uh oh!

Conversation

stsewd commented Jul 10, 2018

Uh oh!

stsewd commented Jul 11, 2018

Uh oh!

stsewd commented Jul 16, 2018

Uh oh!

stsewd 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!

agjohnson 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!

stsewd commented Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

agjohnson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stsewd commented Jul 20, 2018

Uh oh!

agjohnson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stsewd commented Jul 18, 2018 •

edited

Loading