Skip to content

Use default settings for Config object #5056

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 14 commits into from
Jan 22, 2019
Merged

Use default settings for Config object #5056

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
593af88
use default settings
dojutsu-user Jan 2, 2019
da4a1c4
import logging
dojutsu-user Jan 2, 2019
c57a0a4
add variable in settings
dojutsu-user Jan 2, 2019
12e7c35
Add comment
dojutsu-user Jan 2, 2019
f5fea83
move more variables
dojutsu-user Jan 2, 2019
5d7ead4
change name
dojutsu-user Jan 3, 2019
32e7559
Fix test
dojutsu-user Jan 7, 2019
c0dda3b
add improvements
dojutsu-user Jan 15, 2019
1b7912e
Merge branch 'master' into HEAD
dojutsu-user Jan 18, 2019
555f0fb
update test
dojutsu-user Jan 18, 2019
5891db3
Merge branch 'master' into using-defaults
stsewd Jan 21, 2019
b88b167
Update readthedocs/rtd_tests/tests/test_config_integration.py
stsewd Jan 21, 2019
77102a1
Merge branch 'master' into using-defaults
dojutsu-user Jan 22, 2019
7c905c6
Merge branch 'master' into using-defaults
dojutsu-user Jan 22, 2019
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
57 changes: 32 additions & 25 deletions readthedocs/rtd_tests/tests/test_config_integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,38 +75,45 @@ def setUp(self):

@mock.patch('readthedocs.doc_builder.config.load_config')
def test_python_supported_versions_default_image_1_0(self, load_config):
from doc_builder.constants import DOCKER_IMAGE_SETTINGS

load_config.side_effect = create_load()
self.project.container_image = 'readthedocs/build:1.0'
self.project.enable_epub_build = True
self.project.enable_pdf_build = True
self.project.save()
config = load_yaml_config(self.version)
self.assertEqual(load_config.call_count, 1)
load_config.assert_has_calls([
mock.call(

expected_env_config = {
'allow_v2': mock.ANY,
'build': {'image': 'readthedocs/build:1.0'},
'output_base': '',
'name': mock.ANY,
'defaults': {
'install_project': self.project.install_project,
'formats': [
'htmlzip',
'epub',
'pdf'
],
'use_system_packages': self.project.use_system_packages,
'requirements_file': self.project.requirements_file,
'python_version': 2,
'sphinx_configuration': mock.ANY,
'build_image': 'readthedocs/build:1.0',
'doctype': self.project.documentation_type,
},
}

img_settings = DOCKER_IMAGE_SETTINGS.get(self.project.container_image, None)
if img_settings:
expected_env_config.update(img_settings)
expected_env_config['DOCKER_IMAGE_SETTINGS'] = img_settings
Copy link
Member

Choose a reason for hiding this comment

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

I don't follow why we are updating the expected_env_config twice. Why is that?

Copy link
Member Author

Choose a reason for hiding this comment

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

the value of img_settings is: {'python': {'supported_versions': [2, 2.7, 3, 3.4]}}.
and the actual call has both of the values.
Actual Call:

{
    ...
    ...
    'python': {'supported_versions': [2, 2.7, 3, 3.4]},
    ...
    ...
    'DOCKER_IMAGE_SETTINGS': {'python': {'supported_versions': [2, 2.7, 3, 3.4]}},
    ...
    ...
}

Copy link
Member

Choose a reason for hiding this comment

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

Looks like we need to refactor the code to only have/use DOCKER_IMAGES_SETTINGS

Copy link
Member Author

Choose a reason for hiding this comment

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

@stsewd

https://github.com/rtfd/readthedocs.org/blob/a071a81a06edb013c4a8eb91b97563f5cf7e52d6/readthedocs/doc_builder/config.py#L60-L62

I just removed the line number 61 and all the tests pass in local. I am not very familiar with this part of code. So can you please tell me if that is all required? or it will break some things in production?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this was added in #3339. I'm investigating more, but it looks like it was for doing what we are doing now.

Copy link
Member

Choose a reason for hiding this comment

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

I just opened #5116 to have this more clear, after that PR is merged, it should be easier to see what parts of the code remove/replace.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you @stsewd
I will update this PR after merging of #5116


load_config.assert_called_once_with(
path=mock.ANY,
env_config={
'allow_v2': mock.ANY,
'build': {'image': 'readthedocs/build:1.0'},
'output_base': '',
'name': mock.ANY,
'defaults': {
'install_project': self.project.install_project,
'formats': [
'htmlzip',
'epub',
'pdf'
],
'use_system_packages': self.project.use_system_packages,
'requirements_file': self.project.requirements_file,
'python_version': 2,
'sphinx_configuration': mock.ANY,
'build_image': 'readthedocs/build:1.0',
'doctype': self.project.documentation_type,
},
},
),
])
env_config=expected_env_config,
)
self.assertEqual(config.python.version, 2)

@mock.patch('readthedocs.doc_builder.config.load_config')
Expand Down