-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Conversation
readthedocs/config/config.py
Outdated
}, | ||
} | ||
DOCKER_IMAGE = getattr(settings, 'DOCKER_IMAGE', 'readthedocs/build:2.0') | ||
DOCKER_IMAGE_SETTINGS = getattr(settings, 'CONFIG_DOCKER_IMAGE_SETTINGS', {}) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
We could start using the approach in #2140, just a comment, this still needs some approval from another core dev. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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]}},
...
...
}
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Blocking this because of #5116 |
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') |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]}, |
There was a problem hiding this comment.
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
}, | ||
'readthedocs/build:3.0': { | ||
'python': {'supported_versions': [2, 2.7, 3, 3.3, 3.4, 3.5, 3.6]}, | ||
}, |
There was a problem hiding this comment.
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.
@@ -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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3.0
here.
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 |
@dojutsu-user yeah, that's easier to do in another PR |
I have reverted the changes back and updated the test. |
Co-Authored-By: dojutsu-user <vaibhgupt199@gmail.com>
@humitos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, we should refactor v1 to read the python supported versions from settings too, but we can do that in another PR
@stsewd I'm on it! Will send a PR for that soon ;) |
@dojutsu-user thanks for the patience here and for helping us making Read the Docs better! |
Fixes #5055