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

Set full source_file path for default configuration #4379

Merged
merged 1 commit into from Jul 17, 2018

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Jul 17, 2018

This was introduced in #4298. I can't replicate this locally (I think this is something to do with the builds being executed in another server). But debugging this I think I found the problem.

Before #4298 the requirements_file wasn't validated, but now it is. For the validation, we use the dir name of the source_file. This was '' (empty/cwd). To fix this I'm passing the full source_file as the checkout path of the build.

This only affects people using the web interface to set the requirements file.

As workaround users can use a configuration file to set the requirements_file setting

Fix #4378

@stsewd stsewd requested a review from Jul 17, 2018
@@ -68,7 +70,7 @@ def load_yaml_config(version):
config = BuildConfig(
env_config=env_config,
raw_config={},
source_file='empty',
source_file=path.join(checkout_path, 'empty'),
Copy link
Member Author

@stsewd stsewd Jul 17, 2018

In the future, we want to manage this in a less hacky way (I mean the empty file)

Copy link
Contributor

@davidfischer davidfischer Jul 17, 2018

It seems like we'd want to just pass None rather than 'empty' but possibly that is a larger refactor.

@@ -1153,7 +1153,10 @@ def assertArgsStartsWith(self, args, function_mock):
if arg is not mock.ANY:
self.assertTrue(arg_mock.startswith(arg))

def test_install_core_requirements_sphinx(self):
@patch('readthedocs.projects.models.Project.checkout_path')
Copy link
Member Author

@stsewd stsewd Jul 17, 2018

This shouldn't be necessary if we remove

https://github.com/rtfd/readthedocs.org/blob/ebf0987fc8a739b47cd8c0ef9fb6c65be7af4ff4/readthedocs/doc_builder/python_environments.py#L37-L38

I didn't do it here because the changes would be bigger.

@rb2745
Copy link

@rb2745 rb2745 commented Jul 17, 2018

To help validate this change and improve documentation it would help to have a list of configuration parameters with: where they can be provided (web admin interface vs. configuration yaml file), what the default is when not provided either place, and what takes precedence when provided both places.

@AdrianLxM
Copy link

@AdrianLxM AdrianLxM commented Jul 17, 2018

As workaround users can use a configuration file to set the requirements_file setting

Do you have an example for that?

@rb2745
Copy link

@rb2745 rb2745 commented Jul 17, 2018

Yes example configuration file in root repository folder setting requirements_file https://gerrit.onap.org/r/gitweb?p=doc.git;a=blob;f=readthedocs.yml;h=6e6b9af07e6d523a60342e76bf181288bf826c63;hb=HEAD

@AdrianLxM
Copy link

@AdrianLxM AdrianLxM commented Jul 17, 2018

@rb2745 thank you!

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jul 17, 2018

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jul 17, 2018

@rb2745 we take precedence to the settings in the configuration file, if a setting is not given, it defaults to the ones from the web interface. This will change with the v2 of the configuration file and be appropriated documented.

earthgecko added a commit to earthgecko/skyline that referenced this issue Jul 17, 2018
Added as readthedocs in broken as per readthedocs/readthedocs.org#4379
Added:
readthedocs.yml
Copy link
Contributor

@davidfischer davidfischer left a comment

This change seems small enough.

I do believe this warrants a refactor but not right now.

@@ -68,7 +70,7 @@ def load_yaml_config(version):
config = BuildConfig(
env_config=env_config,
raw_config={},
source_file='empty',
source_file=path.join(checkout_path, 'empty'),
Copy link
Contributor

@davidfischer davidfischer Jul 17, 2018

It seems like we'd want to just pass None rather than 'empty' but possibly that is a larger refactor.

@davidfischer davidfischer merged commit 070c322 into readthedocs:master Jul 17, 2018
1 check passed
@stsewd stsewd deleted the fix-base-path-config-v1 branch Jul 17, 2018
@humitos
Copy link
Member

@humitos humitos commented Jul 17, 2018

Re labelled again as hotfix since it's already in production. We will have a release comming out soon, though.

We re-trigger a build of freqgen and it passed: https://readthedocs.org/projects/freqgen/builds/7500721/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants