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

Conversation

Projects
None yet
2 participants
@stsewd
Member

stsewd commented Jul 10, 2018

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

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Jul 11, 2018

Member

Ok, I have ~147 tests to pass

Member

stsewd commented Jul 11, 2018

Ok, I have ~147 tests to pass

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Jul 16, 2018

Member

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.

Member

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.

@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.

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

This comment has been minimized.

@stsewd

stsewd Jul 16, 2018

Member

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

@stsewd

stsewd Jul 16, 2018

Member

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

This comment has been minimized.

@agjohnson

agjohnson Jul 17, 2018

Contributor

For now, yeah.

@agjohnson

agjohnson Jul 17, 2018

Contributor

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)

This comment has been minimized.

@stsewd

stsewd Jul 16, 2018

Member

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

@stsewd

stsewd Jul 16, 2018

Member

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

This comment has been minimized.

@stsewd

stsewd Jul 16, 2018

Member

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

@stsewd

stsewd Jul 16, 2018

Member

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

This comment has been minimized.

@agjohnson

agjohnson Jul 17, 2018

Contributor

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

agjohnson Jul 17, 2018

Contributor

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.

This comment has been minimized.

@stsewd

stsewd Jul 17, 2018

Member

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?

@stsewd

stsewd Jul 17, 2018

Member

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.

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

This comment has been minimized.

@agjohnson

agjohnson Jul 17, 2018

Contributor

For now, yeah.

@agjohnson

agjohnson Jul 17, 2018

Contributor

For now, yeah.

Show outdated Hide outdated readthedocs/config/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)

This comment has been minimized.

@agjohnson

agjohnson Jul 17, 2018

Contributor

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

agjohnson Jul 17, 2018

Contributor

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'

This comment has been minimized.

@agjohnson

agjohnson Jul 17, 2018

Contributor

Noted above, i think this feature flag name should change

@agjohnson

agjohnson Jul 17, 2018

Contributor

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

This comment has been minimized.

@agjohnson

agjohnson Jul 17, 2018

Contributor

Also needs update here

@agjohnson

agjohnson Jul 17, 2018

Contributor

Also needs update here

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Jul 18, 2018

Member

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.

Member

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.

@stsewd stsewd requested a review from agjohnson Jul 18, 2018

@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.

Show outdated Hide outdated readthedocs/config/config.py

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

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Jul 20, 2018

Member

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.

Member

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.

@stsewd stsewd requested a review from agjohnson Jul 23, 2018

@agjohnson

I think these changes look good to merge! 👍

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stsewd stsewd deleted the stsewd:build-config-v2 branch Aug 2, 2018

@stsewd stsewd restored the stsewd:build-config-v2 branch Aug 2, 2018

@stsewd stsewd deleted the stsewd: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