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

Add schema for configuration file with yamale #4084

Merged
merged 30 commits into from
Jun 8, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions readthedocs/rtd_tests/fixtures/spec/v2/schema.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ submodules: include('submodules', required=False)
# Redirects for the current version to be built
# Key/value list, represent redirects of type `type`
# from url -> to url
# Default: null
redirects: map(enum('page'), map(str(), str()), required=False)

---
Expand All @@ -47,9 +48,11 @@ python:
version: enum('2', '2.7', '3', '3.5', '3.6', required=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what to do here. This is dependent on the docker image. Our current latest image (our default image will soon be latest) has 2.7, 3.3, 3.4, 3.5, and 3.6. The 4.0 image has 2.7, 3.5, 3.6, and should probably have 3.7 soon.

I think verifying the python version list is a config parsing level validation, we don't necessarily need to worry about validating across fields as part of this exercise. Perhaps here, for our spec discussion, we just put all possible python versions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps here, for our spec discussion, we just put all possible python versions?

I'd say yes


# The path to the requirements file from the root of the project
# Default: null
requirements: path(required=False)

# Install the project using python setup.py install or pip
# Default: null
install: enum('pip', 'setup.py', required=False)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the first breaking change :), I think is more simple to do this, instead of having a boolean field for each

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, the extra requirements field just works when the project is installed using pip, but setup.py doesn't allow this too? 🤔


# Extra requirements sections to install in addition to the package dependencies
Expand All @@ -60,13 +63,24 @@ python:
# Default: false
system_packages: bool(required=False)
Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't 100% sure if this should belong in the python key

Copy link
Member

Choose a reason for hiding this comment

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

I mostly want this option to go away, but I don't know if we can do it quite yet. I think it can go in python though.


sphinx:
sphinx:
# The path to the conf.py file
# Default: rtd will try to find it
configuration: path(required=False)

# Add the -W option to sphinx-build
# Default: false
fail_on_warning: bool(required=False)

mkdocs:
# Something
# The path to the mkdocs.yml file
# Default: rtd will try to find it
configuration: path(required=False)

# Add the --strict optio to mkdocs build
# Default: false
fail_on_warning: bool(required=False)


submodules:
# List of submodules to be included
Expand Down
137 changes: 131 additions & 6 deletions readthedocs/rtd_tests/tests/test_yml_schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,20 @@

import pytest
from readthedocs.rtdyml import BuildConfig
from readthedocs.rtd_tests.utils import apply_fs
from readthedocs_build.testing import utils


def create_yaml(tmpdir, content):
fs = {
'environment.yml': '',
'mkdocs.yml': '',
'rtd.yml': content,
'docs': {
'conf.py': '',
'requirements.txt': '',
},
}
apply_fs(tmpdir, fs)
utils.apply_fs(tmpdir, fs)
return path.join(tmpdir.strpath, 'rtd.yml')


Expand Down Expand Up @@ -192,7 +193,7 @@ def test_python_version_invalid(tmpdir):
)


def test_no_python_version(tmpdir):
def test_python_version_no_key(tmpdir):
content = '''
version: "2"
python:
Expand All @@ -201,7 +202,7 @@ def test_no_python_version(tmpdir):
assertValidConfig(tmpdir, content)


def test_valid_requirements(tmpdir):
def test_python_requirements(tmpdir):
content = '''
version: "2"
python:
Expand All @@ -210,7 +211,7 @@ def test_valid_requirements(tmpdir):
assertValidConfig(tmpdir, content)


def test_invalid_requirements_file(tmpdir):
def test_python_requirements_invalid(tmpdir):
content = '''
version: "2"
python:
Expand All @@ -223,6 +224,15 @@ def test_invalid_requirements_file(tmpdir):
)


def test_python_requirements_null(tmpdir):
content = '''
version: "2"
python:
requirements: null
'''
assertValidConfig(tmpdir, content)


@pytest.mark.parametrize('value', ['pip', 'setup.py'])
def test_python_install(tmpdir, value):
content = '''
Expand All @@ -247,6 +257,15 @@ def test_python_install_invalid(tmpdir):
)


def test_python_install_null(tmpdir):
content = '''
version: "2"
python:
install: null
'''
assertValidConfig(tmpdir, content)


def test_python_extra_requirements(tmpdir):
content = '''
version: "2"
Expand All @@ -273,6 +292,16 @@ def test_python_extra_requirements_invalid(tmpdir):
)


@pytest.mark.parametrize('value', ['', 'null', '[]'])
Copy link
Member

Choose a reason for hiding this comment

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

Oh, yes!

def test_python_extra_requirements_empty(tmpdir, value):
content = '''
version: "2"
python:
extra_requirements: {value}
'''
assertValidConfig(tmpdir, content.format(value=value))


@pytest.mark.parametrize('value', ['true', 'false'])
def test_python_system_packages(tmpdir, value):
content = '''
Expand Down Expand Up @@ -300,6 +329,15 @@ def test_python_system_packages_invalid(tmpdir, value):
def test_sphinx(tmpdir):
content = '''
version: "2"
sphinx:
configuration: docs/conf.py
'''
assertValidConfig(tmpdir, content)


def test_sphinx_default_value(tmpdir):
content = '''
version: "2"
sphinx:
file: docs/conf.py # Default value for configuration key
'''
Expand All @@ -320,6 +358,84 @@ def test_sphinx_invalid(tmpdir, value):
)


def test_sphinx_fail_on_warning(tmpdir):
content = '''
version: "2"
sphinx:
fail_on_warning: true
'''
assertValidConfig(tmpdir, content)


@pytest.mark.parametrize('value', ['not true', "''", '[]'])
def test_sphinx_fail_on_warning_invalid(tmpdir, value):
content = '''
version: "2"
sphinx:
fail_on_warning: {value}
'''
assertInvalidConfig(
tmpdir,
content.format(value=value),
['is not a bool']
)


def test_mkdocs(tmpdir):
content = '''
version: "2"
mkdocs:
configuration: mkdocs.yml
'''
assertValidConfig(tmpdir, content)


def test_mkdocs_default_value(tmpdir):
content = '''
version: "2"
mkdocs:
file: mkdocs.yml # Default value for configuration key
'''
assertValidConfig(tmpdir, content)


@pytest.mark.parametrize('value', ['2', 'environment.py'])
Copy link
Member

Choose a reason for hiding this comment

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

Why it does fail with environment.py? I think that it's a valid path and means that the environment.py file is at the root level of the repository.

(there are other places where I have the same question --for example, conda.environment)

Copy link
Member Author

Choose a reason for hiding this comment

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

It fails because it doesn't exist in the current file system (there is only an environment.yaml file)

Copy link
Member

Choose a reason for hiding this comment

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

Got it.

Two things here:

  1. I'd use something more descriptive on the values when we are testing something in particular, like non-existent-file.yml for example. In that case, you don't need to read the whole implementation to understand why it will fail
  2. I think the validation should raise two different exceptions. One for something that's not a "valid path" like http://not-a-valid-path.com and another one for /file/not/found

(note that in 2 I used what I proposed in 1 and it's very clear)

Copy link
Member Author

Choose a reason for hiding this comment

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

I go a little crazy with the names on tests p:

I'm not sure how to validate a valid path without doing it in a hacky way or a complicated regex, not sure if it's worth it.

Copy link
Member

Choose a reason for hiding this comment

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

We probably don't want to validate that the file exists on the schema, since any path is valid for schema but we want to validate that the file exists when using the YAML file to build the docs.

As @ericholscher said in another comment, we may want to have these validations at different places/steps.

(although, my suggestion about the names of the invalid options/files/etc is still valid and I think you should follow it :) )

def test_mkdocs_invalid(tmpdir, value):
content = '''
version: "2"
mkdocs:
configuration: {value}
'''
assertInvalidConfig(
tmpdir,
content,
['is not a path']
)


def test_mkdocs_fail_on_warning(tmpdir):
content = '''
version: "2"
mkdocs:
fail_on_warning: true
'''
assertValidConfig(tmpdir, content)


@pytest.mark.parametrize('value', ['not true', "''", '[]'])
def test_mkdocs_fail_on_warning_invalid(tmpdir, value):
content = '''
version: "2"
mkdocs:
fail_on_warning: {value}
'''
assertInvalidConfig(
tmpdir,
content.format(value=value),
['is not a bool']
)


def test_submodules_include(tmpdir):
content = '''
version: "2"
Expand Down Expand Up @@ -374,7 +490,7 @@ def test_redirects(tmpdir):
assertValidConfig(tmpdir, content)


def test_invalid_redirects(tmpdir):
def test_redirects_invalid(tmpdir):
content = '''
version: "2"
redirects:
Expand All @@ -386,3 +502,12 @@ def test_invalid_redirects(tmpdir):
content,
['is not a str']
)


@pytest.mark.parametrize('value', ['', 'null', '{}'])
def test_redirects_empty(tmpdir, value):
content = '''
version: "2"
redirects: {value}
'''
assertValidConfig(tmpdir, content.format(value=value))
15 changes: 0 additions & 15 deletions readthedocs/rtd_tests/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -121,21 +121,6 @@ def make_test_hg():
return directory


def apply_fs(tmpdir, contents):
"""
Create the directory structure specified in ``contents``. It's a dict of
filenames as keys and the file contents as values. If the value is another
dict, it's a subdirectory.
"""
for filename, content in contents.items():
if hasattr(content, 'items'):
apply_fs(tmpdir.mkdir(filename), content)
else:
file = tmpdir.join(filename)
file.write(content)
return tmpdir


def create_user(username, password, **kwargs):
user = new(User, username=username, **kwargs)
user.set_password(password)
Expand Down
15 changes: 12 additions & 3 deletions readthedocs/rtdyml/__init__.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
"""Validator for the RTD configuration file."""

from os import path

import six
Expand All @@ -12,8 +14,12 @@


class PathValidator(Validator):

"""
Docstring
Path validator

Checks if the given value is a string and a existing
file.
"""

tag = 'path'
Expand All @@ -32,19 +38,22 @@ def _is_valid(self, value):

class BuildConfig(object):

"""Wrapper object to validate to configuration file."""

def __init__(self, configuration_file):
self.configuration_file = configuration_file
self.data = yamale.make_data(self.configuration_file)
self.schema = yamale.make_schema(
V2_SCHEMA,
validators=self.get_validators()
validators=self._get_validators()
)

def get_validators(self):
def _get_validators(self):
validators = DefaultValidators.copy()
PathValidator.configuration_file = self.configuration_file
validators[PathValidator.tag] = PathValidator
return validators

def validate(self):
"""Validate the current configuration file."""
return yamale.validate(self.schema, self.data)