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

V2 of the configuration file #4355

Merged
merged 68 commits into from Aug 2, 2018
Merged

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Jul 10, 2018

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

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jul 11, 2018

Ok, I have ~147 tests to pass

This ones have a replace in v2
@stsewd
Copy link
Member Author

@stsewd stsewd commented Jul 16, 2018

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.

Copy link
Member Author

@stsewd stsewd left a comment

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.

},
'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]},
Copy link
Member Author

@stsewd stsewd Jul 16, 2018

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

Copy link
Contributor

@agjohnson agjohnson Jul 17, 2018

For now, yeah.

@@ -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)
Copy link
Member Author

@stsewd stsewd Jul 16, 2018

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

Copy link
Member Author

@stsewd stsewd Jul 16, 2018

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

Copy link
Contributor

@agjohnson agjohnson Jul 17, 2018

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.

Copy link
Member Author

@stsewd stsewd Jul 17, 2018

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?

Copy link
Contributor

@agjohnson agjohnson left a comment

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.

},
'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]},
Copy link
Contributor

@agjohnson agjohnson Jul 17, 2018

For now, yeah.

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

@agjohnson agjohnson Jul 17, 2018

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.

@@ -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)
Copy link
Contributor

@agjohnson agjohnson Jul 17, 2018

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.

@@ -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'
Copy link
Contributor

@agjohnson agjohnson Jul 17, 2018

Noted above, i think this feature flag name should change

@@ -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')),
Copy link
Contributor

@agjohnson agjohnson Jul 17, 2018

Also needs update here

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jul 18, 2018

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.

valid_sphinx_builders = {
'sphinx': 'sphinx',
'htmldir': 'sphinx_htmldir',
'singlehtml': 'sphinx_singlehtml',
Copy link
Member Author

@stsewd stsewd Jul 18, 2018

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.

Copy link
Contributor

@agjohnson agjohnson Jul 19, 2018

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.

Copy link
Member Author

@stsewd stsewd Jul 20, 2018

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

Copy link
Contributor

@agjohnson agjohnson Jul 26, 2018

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?

Copy link
Member Author

@stsewd stsewd Jul 26, 2018

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.

Copy link
Contributor

@agjohnson agjohnson Aug 2, 2018

👍 that works

Copy link
Contributor

@agjohnson agjohnson left a comment

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.

valid_sphinx_builders = {
'sphinx': 'sphinx',
'htmldir': 'sphinx_htmldir',
'singlehtml': 'sphinx_singlehtml',
Copy link
Contributor

@agjohnson agjohnson Jul 19, 2018

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.

@agjohnson agjohnson added this to the YAML File Completion milestone Jul 20, 2018
@stsewd
Copy link
Member Author

@stsewd stsewd commented Jul 20, 2018

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.

Copy link
Contributor

@agjohnson agjohnson left a comment

I think these changes look good to merge! 👍

@agjohnson agjohnson merged commit 5be4cd1 into readthedocs:master Aug 2, 2018
1 check passed
@stsewd stsewd deleted the build-config-v2 branch Aug 2, 2018
@stsewd stsewd restored the build-config-v2 branch Aug 2, 2018
@stsewd stsewd mentioned this pull request Aug 2, 2018
3 tasks
@stsewd stsewd deleted the build-config-v2 branch Aug 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants