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

Use default settings for Config object #5056

Merged
merged 14 commits into from Jan 22, 2019

Conversation

@dojutsu-user
Copy link
Member

@dojutsu-user dojutsu-user commented Jan 2, 2019

Fixes #5055

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Jan 2, 2019

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

What can be done about those?

readthedocs/config/config.py Outdated Show resolved Hide resolved
readthedocs/settings/base.py Outdated Show resolved Hide resolved
},
}
DOCKER_IMAGE = getattr(settings, 'DOCKER_IMAGE', 'readthedocs/build:2.0')
DOCKER_IMAGE_SETTINGS = getattr(settings, 'CONFIG_DOCKER_IMAGE_SETTINGS', {})
Copy link
Member

@humitos humitos Jan 3, 2019

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

@dojutsu-user dojutsu-user Jan 3, 2019

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

@humitos humitos Jan 3, 2019

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

@dojutsu-user dojutsu-user Jan 3, 2019

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

@humitos humitos Jan 7, 2019

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

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
Copy link
Member

@stsewd stsewd commented Jan 8, 2019

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

Copy link
Member

@humitos humitos left a comment

This looks great!

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

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

@humitos humitos Jan 14, 2019

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

Copy link
Member Author

@dojutsu-user dojutsu-user Jan 15, 2019

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

@stsewd stsewd Jan 15, 2019

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

Copy link
Member Author

@dojutsu-user dojutsu-user Jan 15, 2019

@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

@stsewd stsewd Jan 15, 2019

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

@stsewd stsewd Jan 16, 2019

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

@dojutsu-user dojutsu-user Jan 16, 2019

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

readthedocs/config/config.py Outdated Show resolved Hide resolved
readthedocs/rtd_tests/tests/test_config_integration.py Outdated Show resolved Hide resolved
@humitos
Copy link
Member

@humitos humitos commented Jan 14, 2019

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.

@humitos humitos added this to the 2.9 milestone Jan 14, 2019
@humitos humitos requested a review from stsewd Jan 14, 2019
@humitos humitos mentioned this pull request Jan 14, 2019
@stsewd
Copy link
Member

@stsewd stsewd commented Jan 16, 2019

Blocking this because of #5116

@humitos
Copy link
Member

@humitos humitos commented Jan 16, 2019

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

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

@humitos humitos Jan 17, 2019

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

Copy link
Member

@humitos humitos left a comment

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

'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

@humitos humitos Jan 17, 2019

latest is 4.0 now, so

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

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

@humitos humitos Jan 17, 2019

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

has to be added here.

@@ -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'
Copy link
Member

@humitos humitos Jan 17, 2019

3.0 here.

@stsewd
Copy link
Member

@stsewd stsewd commented Jan 17, 2019

I just merged #5116.

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

@humitos
Copy link
Member

@humitos humitos commented Jan 17, 2019

@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
Copy link
Member Author

@dojutsu-user dojutsu-user commented Jan 17, 2019

@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
Copy link
Member

@stsewd stsewd commented Jan 17, 2019

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

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Jan 18, 2019

I have reverted the changes back and updated the test.

@humitos
Copy link
Member

@humitos humitos commented Jan 22, 2019

@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
Copy link
Member Author

@dojutsu-user dojutsu-user commented Jan 22, 2019

@humitos
I have resolved the conflicts.

stsewd
stsewd approved these changes Jan 22, 2019
Copy link
Member

@stsewd stsewd left a comment

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

@stsewd stsewd merged commit 7a54182 into readthedocs:master Jan 22, 2019
1 check passed
@dojutsu-user dojutsu-user deleted the using-defaults branch Jan 22, 2019
@humitos
Copy link
Member

@humitos humitos commented Jan 22, 2019

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

@humitos
Copy link
Member

@humitos humitos commented Jan 22, 2019

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

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Jan 22, 2019

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

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

Successfully merging this pull request may close these issues.

None yet

3 participants