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

Show file tree
Hide file tree
Changes from 5 commits
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
25 changes: 5 additions & 20 deletions readthedocs/config/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from contextlib import contextmanager

import six
from django.conf import settings

from readthedocs.projects.constants import DOCUMENTATION_CHOICES

Expand Down Expand Up @@ -55,28 +56,12 @@
INVALID_KEYS_COMBINATION = 'invalid-keys-combination'
INVALID_KEY = 'invalid-key'

DOCKER_DEFAULT_IMAGE = 'readthedocs/build'
DOCKER_DEFAULT_VERSION = '2.0'
DOCKER_DEFAULT_IMAGE = getattr(settings, 'DOCKER_DEFAULT_IMAGE', 'readthedocs/build')
DOCKER_DEFAULT_VERSION = getattr(settings, 'DOCKER_DEFAULT_VERSION', '2.0')
Copy link
Member

Choose a reason for hiding this comment

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

We should default to 3.0 now that we already deploy/release it.

# These map to corresponding settings in the .org,
# so they haven't been renamed.
DOCKER_IMAGE = '{}:{}'.format(DOCKER_DEFAULT_IMAGE, DOCKER_DEFAULT_VERSION)
DOCKER_IMAGE_SETTINGS = {
'readthedocs/build:1.0': {
'python': {'supported_versions': [2, 2.7, 3, 3.4]},
},
'readthedocs/build:2.0': {
'python': {'supported_versions': [2, 2.7, 3, 3.5]},
},
'readthedocs/build:3.0': {
'python': {'supported_versions': [2, 2.7, 3, 3.3, 3.4, 3.5, 3.6]},
},
'readthedocs/build:stable': {
'python': {'supported_versions': [2, 2.7, 3, 3.3, 3.4, 3.5, 3.6]},
},
'readthedocs/build:latest': {
'python': {'supported_versions': [2, 2.7, 3, 3.3, 3.4, 3.5, 3.6]},
},
}
DOCKER_IMAGE = getattr(settings, 'DOCKER_IMAGE', 'readthedocs/build:2.0')
DOCKER_IMAGE_SETTINGS = getattr(settings, 'CONFIG_DOCKER_IMAGE_SETTINGS', {})
Copy link
Member

Choose a reason for hiding this comment

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

Why CONFIG_ was prepended here?

I suppose it just should be DOCKER_IMAGE_SETTINGS as we are already using in our 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.

I didn't get you.
I prepend the CONFIG_ to make these settings specific to the config.py file.
However, I have pushed the changes, but using DOCKER_IMAGE_SETTINGS as the name is causing one test to fail.

Copy link
Member

Choose a reason for hiding this comment

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

We have been using DOCKER_IMAGE_SETTINGS so we should keep using its name.

Regarding the test, please take a look to see if you can fix it or find the reason why it's failing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Earlier, DOCKER_IMAGE_SETTINGS were not defined in settings/base.py.

Reason for test failure:
since DOCKER_IMAGE_SETTINGS is now defined in settings/base.py, these lines

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

add some more items to the dictionary and hence the Actual Call and Expected Call are not same.

This can be fixed pretty easily in two ways:

  1. Mocking DOCKER_IMAGE_SETTINGS to be empty dictionary for this test.
  2. Adding the missing settings in the Expected Call.

I am +1 on first option and -0 on the second and would like to know that what is a better option?

Copy link
Member

Choose a reason for hiding this comment

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

If the test is not about docker images and python versions, mocking as empty dict is the way to go. On the other, if we want to be sure about some specific value on that dictionary, we should mock it with that specific value.

Changing the expected call with the values of that setting is tricky, because then we will add another docker image and the test will fail again.

Copy link
Member Author

@dojutsu-user dojutsu-user Jan 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Actually the test is related to the docker image and python versions, so i added the the missing key-values in the Expected call which is following the same pattern as in the code in my comment above.



class ConfigError(Exception):
Expand Down
21 changes: 21 additions & 0 deletions readthedocs/settings/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,27 @@ def USE_PROMOS(self): # noqa
# Docker
DOCKER_ENABLE = False
DOCKER_IMAGE = 'readthedocs/build:2.0'
DOCKER_DEFAULT_IMAGE = 'readthedocs/build'
DOCKER_DEFAULT_VERSION = '2.0'
Copy link
Member

Choose a reason for hiding this comment

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

3.0 here.


# Settings for config object
CONFIG_DOCKER_IMAGE_SETTINGS = {
'readthedocs/build:1.0': {
'python': {'supported_versions': [2, 2.7, 3, 3.4]},
},
'readthedocs/build:2.0': {
'python': {'supported_versions': [2, 2.7, 3, 3.5]},
},
'readthedocs/build:3.0': {
'python': {'supported_versions': [2, 2.7, 3, 3.3, 3.4, 3.5, 3.6]},
},
Copy link
Member

Choose a reason for hiding this comment

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

        'readthedocs/build:4.0': {
            'python': {'supported_versions': [2, 2.7, 3, 3.5, 3.6, 3.7]},
        },

has to be added here.

'readthedocs/build:stable': {
'python': {'supported_versions': [2, 2.7, 3, 3.3, 3.4, 3.5, 3.6]},
},
'readthedocs/build:latest': {
'python': {'supported_versions': [2, 2.7, 3, 3.3, 3.4, 3.5, 3.6]},
Copy link
Member

Choose a reason for hiding this comment

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

latest is 4.0 now, so

  • 3.3 and 3.4 should be removed from here
  • 3.7 has to be added

},
}

# All auth
ACCOUNT_ADAPTER = 'readthedocs.core.adapters.AccountAdapter'
Expand Down