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

Add docker image from the YAML config integration #3339

Merged
merged 12 commits into from Dec 28, 2017
46 changes: 38 additions & 8 deletions docs/yaml-config.rst
Expand Up @@ -69,6 +69,39 @@ The file option specified the Conda `environment file`_ to use.

.. note:: Conda is only supported via the YAML file.


build
~~~~~

The ``build`` block configures specific aspects of the documentation build.

.. _yaml_build_image:

build.image
```````````

* Default: ``2.0``
* Options: ``1.0``, ``2.0``, ``latest``
Copy link
Contributor

Choose a reason for hiding this comment

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

With settings re-enabled, this can be pulled from settings. See settings.rst and doc_extensions.py in conf/


The docker image to use for specific builds.
This lets users specify a more experimental builder,
if they want to be on the cutting edge.
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency and clarity:

"docker image" -> "build image"
"builder" -> "build image"


Certain Python versions require a certain builder,
as defined here::
Copy link
Contributor

Choose a reason for hiding this comment

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

Same on "builder" here, builder is an internal term, this refers to the build image instead.


* '1.0': 2, 2.7, 3, 3.4
* '2.0': 2, 2.7, 3, 3.5
* 'latest': 2, 2.7, 3, 3.3, 3.4, 3.5, 3.6
Copy link
Contributor

Choose a reason for hiding this comment

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

You might be able to pull this from settings directly, similar to settings.rst. Though i think it's only doing key name lookups now.

Literal formatting would make this more clear


.. code-block:: yaml

build:
image: latest

python:
version: 3.6

python
~~~~~~

Expand All @@ -85,15 +118,12 @@ This is the version of Python to use when building your documentation. If you
specify only the major version of Python, the highest supported minor version
will be selected.

The supported Python versions depends on the version of the build image your
project is using. The default build image that is used to build documentation
contains support for Python ``2.7`` and ``3.5``.

There is also an image in testing that supports Python versions ``2.7``,
``3.3``, ``3.4``, ``3.5``, and ``3.6``. If you would like access to this build
image, you can sign up for beta access here:
.. warning::

https://goo.gl/forms/AKEoeWHixlzVfqKT2
The supported Python versions depends on the version of the build image your
project is using. The default build image that is used to build documentation
contains support for Python ``2.7`` and ``3.5``.
See the :ref:`yaml_build_image` for more information on supported Python versions.

.. code-block:: yaml

Expand Down
32 changes: 13 additions & 19 deletions readthedocs/doc_builder/config.py
Expand Up @@ -8,8 +8,6 @@
from readthedocs_build.config import load as load_config
from readthedocs_build.config import BuildConfig, ConfigError, InvalidConfig

from .constants import DOCKER_BUILD_IMAGES, DOCKER_IMAGE


class ConfigWrapper(object):

Expand Down Expand Up @@ -59,7 +57,7 @@ def python_interpreter(self):
list(
filter(
lambda x: x < ver + 1,
self._yaml_config.get_valid_python_versions(),
self._yaml_config.PYTHON_SUPPORTED_VERSIONS,
Copy link
Contributor

Choose a reason for hiding this comment

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

Raised issue on similar change in readthedocs-build, will require change here too.

)))
return 'python{0}'.format(ver)

Expand Down Expand Up @@ -108,6 +106,15 @@ def formats(self):
formats += ['pdf']
return formats

@property
def build_image(self):
if self._project.container_image:
# Allow us to override per-project still
assert 'readthedocs/build' in self._project.container_image, (
Copy link
Member

Choose a reason for hiding this comment

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

How does this assert work?

Shouldn't we raise a proper exception like ConfigError or InvalidConfig here?

Copy link
Contributor

Choose a reason for hiding this comment

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

seems we shouldn't need to do validation here at all actually -- container_image is admin only already.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, I know I've set it wrong before in the admin, so like having clarity there. Can definitely raise other errors though :)

'container image must be fully qualified')
return self._project.container_image
return 'readthedocs/build:{}'.format(self._yaml_config['build']['image'])
Copy link
Member

Choose a reason for hiding this comment

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

Do we want the user to just put the last part?

Would it be interesting if I can put something like humitos/rtd:latest and install whatever I want in the docker image? Would it be too dangerous? Would it bring a lot of problems?

Just an idea (probably stupid, but...) / comment...

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want users installing their own images on the server. For private instances, I think the addition of DOCKER_BUILD_IMAGES setting made a lot of sense and serves this purpose.

Also, this shouldn't hard code the build image prefix here. Instead, just do lookups on DOCKER_BUILD_IMAGES setting for full build image keys in validation on readthedocs-build


# Not implemented until we figure out how to keep in sync with the webs.
# Probably needs to be version-specific as well, not project.
# @property
Expand All @@ -126,22 +133,8 @@ def load_yaml_config(version):
parsing consistent between projects.
"""
checkout_path = version.project.checkout_path(version.slug)
env_config = {}

# Get build image to set up the python version validation. Pass in the
# build image python limitations to the loaded config so that the versions
# can be rejected at validation
build_image = DOCKER_BUILD_IMAGES.get(
version.project.container_image,
DOCKER_BUILD_IMAGES.get(DOCKER_IMAGE, None),
)
if build_image:
env_config = {
'python': build_image['python'],
}

try:
sphinx_env_config = env_config.copy()
sphinx_env_config = {}
sphinx_env_config.update({
Copy link
Member

Choose a reason for hiding this comment

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

You can just create the dict at this line instead of .update :)

'output_base': '',
'type': 'sphinx',
Expand All @@ -155,8 +148,9 @@ def load_yaml_config(version):
# This is a subclass of ConfigError, so has to come first
raise
except ConfigError:
# Just fall back to defaults
config = BuildConfig(
env_config=env_config,
env_config={},
raw_config={},
source_file='empty',
source_position=0,
Expand Down
14 changes: 0 additions & 14 deletions readthedocs/doc_builder/constants.py
Expand Up @@ -40,17 +40,3 @@
DOCKER_OOM_EXIT_CODE = 137

DOCKER_HOSTNAME_MAX_LEN = 64

# Build images
DOCKER_BUILD_IMAGES = {
'readthedocs/build:1.0': {
'python': {'supported_versions': [2, 2.7, 3, 3.4]},
},
'readthedocs/build:2.0': {
'python': {'supported_versions': [2, 2.7, 3, 3.5]},
},
'readthedocs/build:latest': {
'python': {'supported_versions': [2, 2.7, 3, 3.3, 3.4, 3.5, 3.6]},
},
}
DOCKER_BUILD_IMAGES.update(getattr(settings, 'DOCKER_BUILD_IMAGES', {}))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think keeping full build image names here on readthedocs-build makes sense, as we don't have to hard code any prefixes.

5 changes: 4 additions & 1 deletion readthedocs/doc_builder/environments.py
Expand Up @@ -277,11 +277,12 @@ class BuildEnvironment(object):
:param environment: shell environment variables
"""

def __init__(self, project=None, version=None, build=None, record=True,
def __init__(self, project=None, version=None, build=None, config=None, record=True,
environment=None):
self.project = project
self.version = version
self.build = build
self.config = config
self.record = record
self.environment = environment or {}

Expand Down Expand Up @@ -490,6 +491,8 @@ def __init__(self, *args, **kwargs):
project_name=self.project.slug,
)[:DOCKER_HOSTNAME_MAX_LEN]
)
if self.config and self.config.build_image:
self.container_image = self.config.build_image
if self.project.container_mem_limit:
self.container_mem_limit = self.project.container_mem_limit
if self.project.container_time_limit:
Expand Down
2 changes: 1 addition & 1 deletion readthedocs/projects/tasks.py
Expand Up @@ -210,7 +210,7 @@ def run_build(self, docker=False, record=True):
env_cls = DockerEnvironment
else:
env_cls = LocalEnvironment
self.build_env = env_cls(project=self.project, version=self.version,
self.build_env = env_cls(project=self.project, version=self.version, config=self.config,
build=self.build, record=record, environment=env_vars)

# Environment used for building code, usually with Docker
Expand Down
48 changes: 0 additions & 48 deletions readthedocs/rtd_tests/tests/test_config_wrapper.py
Expand Up @@ -46,54 +46,6 @@ def setUp(self):
install_project=False, requirements_file='urls.py')
self.version = get(Version, project=self.project)

def test_python_supported_versions_default_image_1_0(self, load_config):
load_config.side_effect = create_load()
self.project.container_image = 'readthedocs/build:1.0'
self.project.save()
config = load_yaml_config(self.version)
self.assertEqual(load_config.call_count, 1)
load_config.assert_has_calls([
mock.call(path=mock.ANY, env_config={
'python': {'supported_versions': [2, 2.7, 3, 3.4]},
'type': 'sphinx',
'output_base': '',
'name': mock.ANY
}),
])
self.assertEqual(config.python_version, 2)

def test_python_supported_versions_image_2_0(self, load_config):
load_config.side_effect = create_load()
self.project.container_image = 'readthedocs/build:2.0'
self.project.save()
config = load_yaml_config(self.version)
self.assertEqual(load_config.call_count, 1)
load_config.assert_has_calls([
mock.call(path=mock.ANY, env_config={
'python': {'supported_versions': [2, 2.7, 3, 3.5]},
'type': 'sphinx',
'output_base': '',
'name': mock.ANY
}),
])
self.assertEqual(config.python_version, 2)

def test_python_supported_versions_image_latest(self, load_config):
load_config.side_effect = create_load()
self.project.container_image = 'readthedocs/build:latest'
self.project.save()
config = load_yaml_config(self.version)
self.assertEqual(load_config.call_count, 1)
load_config.assert_has_calls([
mock.call(path=mock.ANY, env_config={
'python': {'supported_versions': [2, 2.7, 3, 3.3, 3.4, 3.5, 3.6]},
'type': 'sphinx',
'output_base': '',
'name': mock.ANY
}),
])
self.assertEqual(config.python_version, 2)

Copy link
Contributor

Choose a reason for hiding this comment

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

These are some of the tests I was looking for in readthedocs-build. I think these tests should be repaired or moved to readthedocs-build, and this should make sure this logic is preserved.

def test_python_default_version(self, load_config):
load_config.side_effect = create_load()
config = load_yaml_config(self.version)
Expand Down
4 changes: 3 additions & 1 deletion requirements/pip.txt
Expand Up @@ -10,7 +10,9 @@ mkdocs==0.14.0
django==1.9.12
six==1.10.0
future==0.16.0
readthedocs-build==2.0.7
#readthedocs-build==2.0.8
# For testing
git+https://github.com/rtfd/readthedocs-build.git@d93c81c#egg=readthedocs_build

django-tastypie==0.13.0
django-haystack==2.6.0
Expand Down