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

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

Merged
merged 1 commit into from Oct 10, 2018

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Oct 4, 2018

Ref #4354

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

@stsewd stsewd Oct 4, 2018

I'm thinking is never defaulting to a project config on v2.

Copy link
Contributor

@agjohnson agjohnson Oct 4, 2018

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

Copy link
Member Author

@stsewd stsewd Oct 4, 2018

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

@humitos humitos Oct 10, 2018

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

@stsewd stsewd Oct 10, 2018

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

@stsewd stsewd Oct 4, 2018

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

@agjohnson agjohnson Oct 4, 2018

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

@stsewd stsewd Oct 4, 2018

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

Copy link
Contributor

@agjohnson agjohnson Oct 4, 2018

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

# 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

@stsewd stsewd Oct 4, 2018

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

Copy link
Contributor

@agjohnson agjohnson Oct 4, 2018

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

# 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

@stsewd stsewd Oct 4, 2018

setuptools instead of setup.py

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

@stsewd stsewd Oct 4, 2018

Little bug 🐛

Copy link
Contributor

@agjohnson agjohnson Oct 4, 2018

👢 🐛

@stsewd
Copy link
Member Author

@stsewd stsewd commented Oct 4, 2018

I haven't added python.install.[pipenv,package] yet, I want to focus on what we already support first

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Oct 4, 2018

good plan! let's note some of the schema changes in the respective issues so we at least have the schema changes planned out.

Copy link
Contributor

@agjohnson agjohnson left a comment

Looks great! My vote is 👍 on these changes. We can wait for other @rtfd/core to chime in here, but I think we discussed the changes here already in good detail.

Copy link
Member

@humitos humitos left a comment

I think this is OK.

I left a comment about the idea of deprecating the settings from Admin if we force our users, that we should consider and maybe open another issue to tackle it.

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

@humitos humitos Oct 10, 2018

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.

@stsewd stsewd merged commit 3458cde into readthedocs:master Oct 10, 2018
1 check passed
@stsewd stsewd deleted the extend-install-option branch Oct 15, 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

3 participants