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 latest docker images as default #5155

Merged
merged 11 commits into from
Jan 29, 2019
66 changes: 50 additions & 16 deletions readthedocs/config/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import copy
import os
import re
from contextlib import contextmanager

from django.conf import settings
Expand Down Expand Up @@ -146,6 +147,8 @@ class BuildConfigBase:
'mkdocs',
'submodules',
]

default_build_image = DOCKER_DEFAULT_VERSION
Copy link
Member

Choose a reason for hiding this comment

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

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

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 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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Using latest was decided in #4084 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

version = None

def __init__(self, env_config, raw_config, source_file):
Expand Down Expand Up @@ -246,6 +249,40 @@ def python_full_version(self):
)
return ver

@property
def valid_build_images(self):
"""
Return all the valid Docker image choices for ``build.image`` option.

The user can use any of this values in the YAML file. These values are
the keys of ``DOCKER_IMAGE_SETTINGS`` Django setting (without the
``readthedocs/build`` part) plus ``stable`` and ``latest``.
"""
images = {'stable', 'latest'}
for k in DOCKER_IMAGE_SETTINGS:
_, version = k.split(':')
if re.fullmatch(r'^[\d\.]+$', version):
images.add(version)
return images

def get_valid_python_versions_for_image(self, build_image):
"""
Return all the valid Python versions for a Docker image.

The Docker image (``build_image``) has to be its complete name, already
validated: ``readthedocs/build:4.0``, not just ``4.0``.

Returns supported versions for the ``DOCKER_DEFAULT_VERSION`` if not
``build_image`` found.
"""

if build_image not in DOCKER_IMAGE_SETTINGS:
build_image = '{}:{}'.format(
DOCKER_DEFAULT_IMAGE,
self.default_build_image,
)
return DOCKER_IMAGE_SETTINGS[build_image]['python']['supported_versions']

def as_dict(self):
config = {}
for name in self.PUBLIC_ATTRIBUTES:
Expand All @@ -268,18 +305,23 @@ class BuildConfigV1(BuildConfigBase):
'"python.extra_requirements" section must be a list.'
)

PYTHON_SUPPORTED_VERSIONS = [2, 2.7, 3, 3.5]
DOCKER_SUPPORTED_VERSIONS = ['1.0', '2.0', 'latest']

version = '1'

def get_valid_python_versions(self):
"""Get all valid python versions."""
"""
Return all valid Python versions.

.. note::

It does not take current build image used into account.
"""
try:
return self.env_config['python']['supported_versions']
except (KeyError, TypeError):
pass
return self.PYTHON_SUPPORTED_VERSIONS
versions = set()
for _, options in DOCKER_IMAGE_SETTINGS.items():
versions = versions.union(options['python']['supported_versions'])
return versions

def get_valid_formats(self): # noqa
"""Get all valid documentation formats."""
Expand Down Expand Up @@ -339,7 +381,7 @@ def validate_build(self):
with self.catch_validation_error('build'):
build['image'] = validate_choice(
str(_build['image']),
self.DOCKER_SUPPORTED_VERSIONS,
self.valid_build_images,
)
if ':' not in build['image']:
# Prepend proper image name to user's image name
Expand Down Expand Up @@ -577,8 +619,6 @@ class BuildConfigV2(BuildConfigBase):

version = '2'
valid_formats = ['htmlzip', 'pdf', 'epub']
valid_build_images = ['1.0', '2.0', '3.0', 'stable', 'latest']
default_build_image = 'latest'
valid_install_method = [PIP, SETUPTOOLS]
valid_sphinx_builders = {
'html': 'sphinx',
Expand Down Expand Up @@ -793,13 +833,7 @@ def get_valid_python_versions(self):
This should be called after ``validate_build()``.
"""
build_image = self.build.image
if build_image not in DOCKER_IMAGE_SETTINGS:
build_image = '{}:{}'.format(
DOCKER_DEFAULT_IMAGE,
self.default_build_image,
)
python = DOCKER_IMAGE_SETTINGS[build_image]['python']
return python['supported_versions']
return self.get_valid_python_versions_for_image(build_image)

def validate_doc_types(self):
"""
Expand Down
12 changes: 6 additions & 6 deletions readthedocs/config/tests/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -363,8 +363,8 @@ def test_it_validates_wrong_type_right_value(self):
)
build.validate()
assert build.python.version == 3
assert build.python_interpreter == 'python3.5'
assert build.python_full_version == 3.5
assert build.python_interpreter == 'python3.7'
assert build.python_full_version == 3.7

def test_it_validates_env_supported_versions(self):
build = get_build_config(
Expand Down Expand Up @@ -483,7 +483,7 @@ def test_it_fails_if_build_is_invalid_option(self, tmpdir):
apply_fs(tmpdir, yaml_config_dir)
build = BuildConfigV1(
{},
{'build': {'image': 3.0}},
{'build': {'image': 3.2}},
source_file=str(tmpdir.join('readthedocs.yml')),
)
with raises(InvalidConfig) as excinfo:
Expand Down Expand Up @@ -513,7 +513,7 @@ def test_it_works_on_python_validation(self, tmpdir):
{},
{
'build': {'image': 'latest'},
'python': {'version': '3.3'},
'python': {'version': '3.6'},
},
source_file=str(tmpdir.join('readthedocs.yml')),
)
Expand All @@ -538,7 +538,7 @@ def test_default(self, tmpdir):
source_file=str(tmpdir.join('readthedocs.yml')),
)
build.validate()
assert build.build.image == 'readthedocs/build:2.0'
assert build.build.image == 'readthedocs/build:latest'

@pytest.mark.parametrize(
'image', ['latest', 'readthedocs/build:3.0', 'rtd/build:latest'],
Expand Down Expand Up @@ -712,7 +712,7 @@ def test_as_dict(tmpdir):
'use_system_site_packages': False,
},
'build': {
'image': 'readthedocs/build:2.0',
'image': 'readthedocs/build:latest',
},
'conda': None,
'sphinx': {
Expand Down
4 changes: 2 additions & 2 deletions readthedocs/rtd_tests/tests/test_config_integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ def test_python_supported_versions_image_latest(self, load_config):
config = load_yaml_config(self.version)
self.assertEqual(
config.get_valid_python_versions(),
[2, 2.7, 3, 3.3, 3.4, 3.5, 3.6],
[2, 2.7, 3, 3.5, 3.6, 3.7],
)

@mock.patch('readthedocs.doc_builder.config.load_config')
Expand Down Expand Up @@ -507,7 +507,7 @@ def test_python_version(self, checkout_path, tmpdir):

config = self.get_update_docs_task().config
assert config.python.version == 3
assert config.python_full_version == 3.6
assert config.python_full_version == 3.7

@patch('readthedocs.doc_builder.environments.BuildEnvironment.run')
def test_python_install_requirements(self, run, checkout_path, tmpdir):
Expand Down
15 changes: 9 additions & 6 deletions readthedocs/settings/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ def USE_PROMOS(self): # noqa
# Docker
DOCKER_ENABLE = False
DOCKER_DEFAULT_IMAGE = 'readthedocs/build'
DOCKER_DEFAULT_VERSION = '2.0'
DOCKER_DEFAULT_VERSION = 'latest'
DOCKER_IMAGE = '{}:{}'.format(DOCKER_DEFAULT_IMAGE, DOCKER_DEFAULT_VERSION)
DOCKER_IMAGE_SETTINGS = {
'readthedocs/build:1.0': {
Expand All @@ -286,14 +286,17 @@ def USE_PROMOS(self): # noqa
'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]},
'readthedocs/build:4.0': {
'python': {'supported_versions': [2, 2.7, 3, 3.5, 3.6, 3.7]},
},
}

# Alias tagged via ``docker tag`` on the build servers
DOCKER_IMAGE_SETTINGS.update({
'readthedocs/build:stable': DOCKER_IMAGE_SETTINGS.get('readthedocs/build:3.0'),
'readthedocs/build:latest': DOCKER_IMAGE_SETTINGS.get('readthedocs/build:4.0'),
})

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