Use default settings for Config object #5056

dojutsu-user · 2019-01-02T13:56:27Z

Fixes #5055

dojutsu-user · 2019-01-02T14:00:39Z

I am a little unclear about the comments #1, #2.

What can be done about those?

readthedocs/config/config.py

readthedocs/settings/base.py

humitos · 2019-01-03T09:55:37Z

readthedocs/config/config.py

-    },
-}
+DOCKER_IMAGE = getattr(settings, 'DOCKER_IMAGE', 'readthedocs/build:2.0')
+DOCKER_IMAGE_SETTINGS = getattr(settings, 'CONFIG_DOCKER_IMAGE_SETTINGS', {})


Why CONFIG_ was prepended here?

I suppose it just should be DOCKER_IMAGE_SETTINGS as we are already using in our settings.

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.

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.

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:

Mocking DOCKER_IMAGE_SETTINGS to be empty dictionary for this test.

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?

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.

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.

stsewd · 2019-01-08T02:01:17Z

We could start using the approach in #2140, just a comment, this still needs some approval from another core dev.

humitos

This looks great!

It needs some very basic changes and we are ready to merge it, I guess.

humitos · 2019-01-14T18:41:28Z

readthedocs/rtd_tests/tests/test_config_integration.py

+        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


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

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]}}, ... ... }

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

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

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

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.

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

readthedocs/config/config.py

readthedocs/rtd_tests/tests/test_config_integration.py

humitos · 2019-01-14T18:45:48Z

Considering that we releasing a new version of our Docker image, we need this PR merged sooner than later, so we can update the settings to match the new docker image release.

stsewd · 2019-01-16T16:00:28Z

Blocking this because of #5116

humitos · 2019-01-16T16:06:42Z

All my suggestions/requests were added/fixed. The only missing thing is merging #5116 and adapt solve any confict if there is one.

humitos · 2019-01-17T15:57:31Z

readthedocs/config/config.py

-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')


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

humitos

We need to update the code to the latest docker images released.

humitos · 2019-01-17T15:58:57Z

readthedocs/settings/base.py

+            '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]},


latest is 4.0 now, so

3.3 and 3.4 should be removed from here

3.7 has to be added

humitos · 2019-01-17T16:00:03Z

readthedocs/settings/base.py

+        },
+        'readthedocs/build:3.0': {
+            'python': {'supported_versions': [2, 2.7, 3, 3.3, 3.4, 3.5, 3.6]},
+        },


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

has to be added here.

humitos · 2019-01-17T16:00:19Z

readthedocs/settings/base.py

@@ -268,7 +268,26 @@ 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'


3.0 here.

stsewd · 2019-01-17T17:01:29Z

I just merged #5116.

@humitos can we do that in another PR? Because we'll need to change more than that.

humitos · 2019-01-17T17:21:28Z

@humitos can we do that in another PR? Because we'll need to change more than that.

OK! I'd like to not need to define these things in many places. My idea with the original issue was to centralize all these setting in only one place so it's easier to maintain and to make a docker release. Like, just edit the settings.py file and it's done.

dojutsu-user · 2019-01-17T18:30:27Z

@humitos
Updating the PR makes the tests fail.
I agree with @stsewd that a new PR should be created for the updation as there are many tests failing.

Should I revert back the changes? or fix the tests in the PR only?

stsewd · 2019-01-17T20:30:47Z

@dojutsu-user yeah, that's easier to do in another PR

dojutsu-user · 2019-01-18T10:45:28Z

I have reverted the changes back and updated the test.

readthedocs/rtd_tests/tests/test_config_integration.py

Co-Authored-By: dojutsu-user <[email protected]>

humitos · 2019-01-22T17:18:45Z

@humitos can we do that in another PR? Because we'll need to change more than that.

I just opened #5153 to track this once this PR gets merged. We just need to solve the conflicts and merge it.

dojutsu-user · 2019-01-22T17:40:27Z

@humitos
I have resolved the conflicts.

stsewd

Looks good, we should refactor v1 to read the python supported versions from settings too, but we can do that in another PR

https://github.com/rtfd/readthedocs.org/blob/3304193202a388720b6093915f65a19f17f6deeb/readthedocs/config/config.py#L281

humitos · 2019-01-22T18:06:54Z

@stsewd I'm on it! Will send a PR for that soon ;)

humitos · 2019-01-22T18:07:18Z

@dojutsu-user thanks for the patience here and for helping us making Read the Docs better!

dojutsu-user · 2019-01-22T18:08:49Z

@humitos
@stsewd
Thanks to you both for guiding me on this.

dojutsu-user added 2 commits January 2, 2019 18:46

use default settings

593af88

import logging

da4a1c4

dojutsu-user added 2 commits January 2, 2019 21:53

add variable in settings

c57a0a4

Add comment

12e7c35

stsewd reviewed Jan 2, 2019

View reviewed changes

readthedocs/config/config.py Outdated Show resolved Hide resolved

move more variables

f5fea83

stsewd reviewed Jan 3, 2019

View reviewed changes

readthedocs/settings/base.py Outdated Show resolved Hide resolved

humitos reviewed Jan 3, 2019

View reviewed changes

dojutsu-user added 2 commits January 3, 2019 18:41

change name

5d7ead4

Fix test

32e7559

humitos requested changes Jan 14, 2019

View reviewed changes

humitos added this to the 2.9 milestone Jan 14, 2019

humitos added the Accepted Accepted issue on our roadmap label Jan 14, 2019

humitos requested a review from stsewd January 14, 2019 19:59

humitos mentioned this pull request Jan 14, 2019

Config file v2 docs #4451

Merged

add improvements

c0dda3b

stsewd added the Status: blocked Issue is blocked on another issue label Jan 16, 2019

humitos approved these changes Jan 16, 2019

View reviewed changes

humitos reviewed Jan 17, 2019

View reviewed changes

humitos requested changes Jan 17, 2019

View reviewed changes

humitos mentioned this pull request Jan 17, 2019

Discrepancy between latest build image #5124

Closed

stsewd removed the Status: blocked Issue is blocked on another issue label Jan 17, 2019

dojutsu-user added 2 commits January 18, 2019 15:51

Merge branch 'master' into HEAD

1b7912e

update test

555f0fb

Merge branch 'master' into using-defaults

5891db3

stsewd reviewed Jan 21, 2019

View reviewed changes

readthedocs/rtd_tests/tests/test_config_integration.py Outdated Show resolved Hide resolved

stsewd and others added 2 commits January 21, 2019 23:19

Update readthedocs/rtd_tests/tests/test_config_integration.py

b88b167

Co-Authored-By: dojutsu-user <[email protected]>

Merge branch 'master' into using-defaults

77102a1

humitos mentioned this pull request Jan 22, 2019

Update default docker settings #5153

Closed

humitos approved these changes Jan 22, 2019

View reviewed changes

Merge branch 'master' into using-defaults

7c905c6

stsewd approved these changes Jan 22, 2019

View reviewed changes

stsewd merged commit 7a54182 into readthedocs:master Jan 22, 2019

dojutsu-user deleted the using-defaults branch January 22, 2019 18:04

Use default settings for Config object #5056

Use default settings for Config object #5056

Conversation

dojutsu-user commented Jan 2, 2019

dojutsu-user commented Jan 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dojutsu-user Jan 7, 2019 • edited Loading

Choose a reason for hiding this comment

stsewd commented Jan 8, 2019

humitos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos commented Jan 14, 2019

stsewd commented Jan 16, 2019

humitos commented Jan 16, 2019

Choose a reason for hiding this comment

humitos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd commented Jan 17, 2019

humitos commented Jan 17, 2019

dojutsu-user commented Jan 17, 2019 • edited Loading

stsewd commented Jan 17, 2019

dojutsu-user commented Jan 18, 2019

humitos commented Jan 22, 2019

dojutsu-user commented Jan 22, 2019

stsewd left a comment

Choose a reason for hiding this comment

humitos commented Jan 22, 2019

humitos commented Jan 22, 2019

dojutsu-user commented Jan 22, 2019

dojutsu-user Jan 7, 2019 •

edited

Loading

dojutsu-user commented Jan 17, 2019 •

edited

Loading