Use latest docker images as default #5155

humitos · 2019-01-22T19:07:06Z

This PR defines latest Docker images released as default in our Django settings.

I had to refactor some things from the Config objects (v1 and v2) because they were hard-coding some values internally in the class. While doing this, I found that v2 depends on the Docker image used when returning the valid Python versions (get_valid_python_version) but v1 does not --I left the behavior as it was, though. As I added the new python supported values as valid options for v1, the default Python version returned when using 3 is 3.7, because latest is the default image used when no specified.

Summary,

latest is the default image for YAML v1 and v2
latest is an alias for 4.0 (at this moment)
when python.version==3 is specified, 3.7 is used
no rcX images are allowed to be specified in the YAML file
YAML v1 and v2 accept the same images: 1.0, 2.0, 3.0, 4.0, stable and latest
the images accepted are defined in DOCKER_IMAGE_SETTINGS setting
the default image used is defined in DOCKER_DEFAULT_VERSION
all the hardcoded images from Config clases are removed and only the settings are used

stsewd · 2019-01-22T23:04:50Z

readthedocs/config/config.py

@@ -146,6 +146,8 @@ class BuildConfigBase:
        'mkdocs',
        'submodules',
    ]
+    valid_build_images = [k.split(':')[1] for k in DOCKER_IMAGE_SETTINGS]
+    default_build_image = DOCKER_DEFAULT_VERSION


The default build image are different from v1 and v2. v1 uses 2.0, v2 uses latest.

🤔 I think both should use stable as default image.

Having different image default for v1 and v2 is confusing. Also, defaulting to a fixed version like 2.0 will make us to be tied to that version. Besides, 2.0 is deprecated.

On the other hand, I think that defaulting to latest is not a good practice since it may be not tested enough to use it widely as default.

We will be changing the builds for users using v1, we shouldn't change the spec.

Using latest was decided in #4084 (comment)

We can default to latest as that comment says. I don't agree, but it's not a huge problem, though.

Regarding default to 2.0 for v1 is not a good decision to me. I'd like to change that to stable (this is a non-breaking change and actually is more stable). Right now, we are defaulting to ConfigV1 for new projects, and ConfigV1 is defaulting to 2.0, so in the end, all new projects will be using a 2.0 which is a deprecated build image.

I'm -1 on defaulting to a deprecated Docker build image for new projects.

If there isn't a technical limitation to why we can't use the same image for both, I agree that both should point to the same image.

However, I don't think that image should be stable by default. Everyone should be using latest now, or 3.0 by default. Not continue to use 2.0. The stable image was meant as a way for users to opt into an older image if they are having serious problems with the newer image.

To clarify, terminology here has swapped, as per our conversation on docker image versioning. latest doesn't mean bleeding edge to us, that is what RCs are. latest is the most recent image that we have vetted and consider production ready.

OK.

stable is 3.0 at this moment. Anyways, using latest should not be a problem and will keep our projects up to date, but we will need to careful when releasing new images, since the change will affect the projects immediately.

Considering that we have a feature flag now to use testing, this will allow us to test RCs easily.

If there isn't a technical limitation to why we can't use the same image for both, I agree that both should point to the same image.

I think there isn't a technical limitation, but I'm not 100% sure yet.

I will default both to latest an test locally how it behaves as a first step.

stsewd · 2019-01-22T23:06:42Z

readthedocs/config/config.py

@@ -146,6 +146,8 @@ class BuildConfigBase:
        'mkdocs',
        'submodules',
    ]
+    valid_build_images = [k.split(':')[1] for k in DOCKER_IMAGE_SETTINGS]


Each version has different valid images, v1 accepts number versions, v2 only latest and stable (from the user side).

Means we can still override them from the admin

Mmm... this is interesting. I wasn't aware.

v2 only latest and stable (from the user side).

Isn't this line saying that 3.0 and other numbered versions are supported also: https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/config/config.py#L580

Yeah, but they aren't documented. I'm not sure if we will allow users to specify that from the config file, it wasn't clear to me.

From what I linked in the comment above, I guess no

humitos · 2019-01-23T17:52:17Z

I made the changes that we discussed on the chat and I think this is ready for review.

stsewd · 2019-01-23T17:55:11Z

readthedocs/config/config.py

@@ -146,6 +147,14 @@ class BuildConfigBase:
        'mkdocs',
        'submodules',
    ]
+
+    valid_build_images = {'stable', 'latest'}
+    for k in DOCKER_IMAGE_SETTINGS:


We should put this in a function or property

stsewd

CI failure is only because of the linter

Remove all the hardcoded valid options/choices from the Config objects and get them from Django settings instead.

V1 reads the versions from DOCKER_IMAGE_SETTINGS and keeps only the numbered ones. V2 only accepts `stable` and `latest` (they are hardcoded because they don't change).

YAML v1 and v2 files allow the same Docker build images now, - 1.0 - 2.0 - 3.0 - 4.0 - stable - latest Any new image added to DOCKER_IMAGE_SETTINGS will be automatically picked as accepted for both YAML files.

By making `latest` as default, and considering that this will be `4.0` which has python `3.7`, the default python selected when using `3` will be `3.7`.

humitos · 2019-01-25T16:16:16Z

I just fixed the linting issue.

This PR is ready to be merged from my point of view. I triggered some builds locally and they worked as expected when using the default values. I loved that @stsewd created so many tests around this.

ericholscher

Looks good. Let's ship it :)

humitos added the PR: work in progress Pull request is not ready for full review label Jan 22, 2019

humitos requested review from stsewd and a team January 22, 2019 19:07

humitos force-pushed the humitos/latest-docker-images branch from c18bf2b to 9041e4a Compare January 22, 2019 19:30

stsewd reviewed Jan 22, 2019

View reviewed changes

agjohnson added the Needed: design decision A core team decision is required label Jan 23, 2019

agjohnson added this to the 2.9 milestone Jan 23, 2019

humitos removed Needed: design decision A core team decision is required PR: work in progress Pull request is not ready for full review labels Jan 23, 2019

humitos requested a review from a team January 23, 2019 17:51

stsewd reviewed Jan 23, 2019

View reviewed changes

humitos mentioned this pull request Jan 23, 2019

Set python3 as default interpreter #3581

Merged

humitos force-pushed the humitos/latest-docker-images branch from 5749259 to 320ea9f Compare January 23, 2019 18:14

stsewd approved these changes Jan 24, 2019

View reviewed changes

agjohnson modified the milestones: 2.9, 3.2 Jan 25, 2019

agjohnson added Improvement Minor improvement to code Accepted Accepted issue on our roadmap labels Jan 25, 2019

humitos added 11 commits January 25, 2019 17:10

Default on Docker image: readthedocs/build:3.0

7cc7dda

Include new Docker images in settings

df9ef54

Use DOCKER_* settings for config validation

293c4b8

Remove all the hardcoded valid options/choices from the Config objects and get them from Django settings instead.

Accept numbered versions for V1 and named ones for V2

9bb3ced

V1 reads the versions from DOCKER_IMAGE_SETTINGS and keeps only the numbered ones. V2 only accepts `stable` and `latest` (they are hardcoded because they don't change).

Allow all numbered versions plus stable and latest as valid images

ed9eee8

YAML v1 and v2 files allow the same Docker build images now, - 1.0 - 2.0 - 3.0 - 4.0 - stable - latest Any new image added to DOCKER_IMAGE_SETTINGS will be automatically picked as accepted for both YAML files.

Do not accept testing or 5.0rc1 as valid image

2bf0f88

Default Docker image to latest in YAML v1 and v2

ee64002

By making `latest` as default, and considering that this will be `4.0` which has python `3.7`, the default python selected when using `3` will be `3.7`.

Make test pass the decisions made

b829d70

Lint issue fixed

4cd9133

Make valid_build_images a @Property

caaff03

Lint

a7770c0

humitos force-pushed the humitos/latest-docker-images branch from 8126b74 to a7770c0 Compare January 25, 2019 16:16

ericholscher approved these changes Jan 29, 2019

View reviewed changes

ericholscher merged commit 04fd0c2 into master Jan 29, 2019

delete-merged-branch bot deleted the humitos/latest-docker-images branch January 29, 2019 20:26

stsewd mentioned this pull request Jan 31, 2019

Update default docker settings #5153

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use latest docker images as default #5155

Use latest docker images as default #5155

humitos commented Jan 22, 2019 •

edited

Loading

stsewd Jan 22, 2019

humitos Jan 23, 2019

stsewd Jan 23, 2019

humitos Jan 23, 2019

agjohnson Jan 23, 2019

agjohnson Jan 23, 2019

humitos Jan 23, 2019

humitos Jan 23, 2019

stsewd Jan 22, 2019

stsewd Jan 22, 2019

humitos Jan 23, 2019

humitos Jan 23, 2019

stsewd Jan 23, 2019

stsewd Jan 23, 2019

humitos commented Jan 23, 2019

stsewd Jan 23, 2019

stsewd left a comment

humitos commented Jan 25, 2019

ericholscher left a comment

Use latest docker images as default #5155

Use latest docker images as default #5155

Conversation

humitos commented Jan 22, 2019 • edited Loading

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

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 23, 2019

Choose a reason for hiding this comment

stsewd left a comment

Choose a reason for hiding this comment

humitos commented Jan 25, 2019

ericholscher left a comment

Choose a reason for hiding this comment

humitos commented Jan 22, 2019 •

edited

Loading