Skip to content

Extend install option from config file (v2, schema only) #4732

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 10, 2018
Merged

Extend install option from config file (v2, schema only) #4732

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 18 additions & 10 deletions readthedocs/rtd_tests/fixtures/spec/v2/schema.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,22 +53,30 @@ python:
# Default: '3'
version: enum('2', '2.7', '3', '3.3', '3.4', '3.5', '3.6', required=False)

# Give the virtual environment access to the global site-packages dir
# Default: false | project config
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'm thinking is never defaulting to a project config on v2.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by that? Are we not using the project model config here?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are, but I think the purpose of v2 is to replace all these configurations from the web admin, that way we can deprecate the admin panel faster.

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of "do not default to values from admin in v2". Although, if we are deprecating this in a strict way like this, we will need to write a migrate guide or, even better, write a tool that produces the YAML file from the current Admin options.

Otherwise, we will be forcing all of our users that just want one new option from v2 to pass over a more complex and manual process.

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'm doing a chagelog/guide to help the migration from v1 or web #4451. We have discussed about suggesting a v2 config file, but we never created an issue. I'm creating one right now.

system_packages: bool(required=False)

# Installation of packages and requiremens
install: list(any(include('python-install-requirements'), include('python-install')), 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.

We don't install anything by default, are we ok with that? Previously we were installing from requirements.txt if we found one in the project.

Copy link
Contributor

Choose a reason for hiding this comment

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

hrm, good question. i sort of dislike some of our magic like this, but I'm sure there are a lot of projects working because of this magic right now. With the config file available, new projects have a more reproducible and easy way of adding a requirements file. Maybe we apply a feature flag in a migration here and let old projects continue to magic find requirements.txt?

That's my vote at least.

Copy link
Member Author

Choose a reason for hiding this comment

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

But this will only affect people using a v2 config file, people using v1 or none, will keep the previous behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, i think you're right, we can change the behavior between the two specs. I'm fine dropping the autofind requirements option for v2


python-install-requirements:
# The path to the requirements file from the root of the project
# Default: null | project config
requirements: path(required=False)
requirements: path()
Copy link
Member Author

Choose a reason for hiding this comment

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

No more hacks with requirements: '' (empty string) to avoid installing from requirements (we can keep that behaviour for v1)

Copy link
Contributor

Choose a reason for hiding this comment

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

oh yeah, v1 could just have the magic find for requirements files, yeah. v2 expects explicit requirements declarations. I'm 👍 on this plan.


# Install the project using python setup.py install or pip
# Default: null | project config
install: enum('pip', 'setup.py', required=False)
python-install:
# The path to the project to be installed
path: path()

# Extra requirements sections to install in addition to the package dependencies
# Install using python setup.py install or pip
# Default: pip
method: enum('pip', 'setuptools', 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.

setuptools instead of setup.py


# Extra requirements sections to install in addition to the package dependencies.
# Only valid when `method` is `pip`.
# Default: []
extra_requirements: list(str(), required=False)

# Give the virtual environment access to the global site-packages dir
# Default: false | project config
system_packages: bool(required=False)

sphinx:
# The builder type for the sphinx documentation
# Default: 'html'
Expand Down
56 changes: 20 additions & 36 deletions readthedocs/rtd_tests/tests/test_build_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ def assertInvalidConfig(tmpdir, content, msgs=()):
with pytest.raises(ValueError) as excinfo:
validate_schema(file)
for msg in msgs:
msg in str(excinfo.value)
assert msg in str(excinfo.value)
Copy link
Member Author

Choose a reason for hiding this comment

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

Little bug 🐛

Copy link
Contributor

Choose a reason for hiding this comment

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

👢 🐛



def test_minimal_config(tmpdir):
Expand Down Expand Up @@ -250,16 +250,18 @@ def test_python_requirements(tmpdir):
content = '''
version: "2"
python:
requirements: docs/requirements.txt
install:
- requirements: docs/requirements.txt
'''
assertValidConfig(tmpdir, content)


def test_python_requirements_invalid(tmpdir):
def test_python_install_requirements(tmpdir):
content = '''
version: "2"
python:
requirements: 23
install:
- requirements: 23
'''
assertInvalidConfig(
tmpdir,
Expand All @@ -268,22 +270,15 @@ def test_python_requirements_invalid(tmpdir):
)


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


@pytest.mark.parametrize('value', ['pip', 'setup.py'])
@pytest.mark.parametrize('value', ['pip', 'setuptools'])
def test_python_install(tmpdir, value):
content = '''
version: "2"
python:
version: "3.6"
install: {value}
install:
- path: .
method: {value}
'''
assertValidConfig(tmpdir, content.format(value=value))

Expand All @@ -297,7 +292,7 @@ def test_python_install_invalid(tmpdir):
assertInvalidConfig(
tmpdir,
content,
["python.install: 'guido' not in"]
["python.install: 'guido' is not a list"]
)


Expand All @@ -310,38 +305,27 @@ def test_python_install_null(tmpdir):
assertValidConfig(tmpdir, content)


def test_python_extra_requirements(tmpdir):
def test_python_install_extra_requirements(tmpdir):
content = '''
version: "2"
python:
extra_requirements:
- test
- dev
install:
- path: .
extra_requirements:
- test
- dev
'''
assertValidConfig(tmpdir, content)


def test_python_extra_requirements_invalid(tmpdir):
content = '''
version: "2"
python:
extra_requirements:
- 1
- dev
'''
assertInvalidConfig(
tmpdir,
content,
["'1' is not a str"]
)


@pytest.mark.parametrize('value', ['', 'null', '[]'])
def test_python_extra_requirements_empty(tmpdir, value):
content = '''
version: "2"
python:
extra_requirements: {value}
install:
- path: .
extra_requirements: {value}
'''
assertValidConfig(tmpdir, content.format(value=value))

Expand Down