Add the ability to add a docker config image #33

ericholscher · 2017-11-30T19:49:34Z

This removes the old way of passing in the docker & python support info, and keeps it inside rtd-build. This is needed so we can verify the values the user has passed in for the docker image and python version are valid together!

humitos · 2017-11-30T23:30:15Z

readthedocs_build/config/config.py

@@ -30,6 +26,18 @@
 TYPE_REQUIRED = 'type-required'
 PYTHON_INVALID = 'python-invalid'

+DOCKER_BUILD_IMAGES = {


Make this updatable from a setting as it was, so we can define a custome DOCKER_BUILD_IMAGES in our development instance.

hmm, good call. Hadn't thought of that specific use.

+1 on a setting, we shouldn't change that behavior. This is important for all non-prod instances.

humitos · 2017-11-30T23:38:55Z

readthedocs_build/config/test_config.py

+            source_file=str(tmpdir.join('readthedocs.yml')),
+            source_position=0)
+        build.validate_build()
+        assert build['build']['image'] == 'latest'


shouldn't we check here that 3.6 was the version selected by default?

I don't think we're opinionated about this yet. We should be eventually though.

+1 on checks for default version though, that seems to be missing from test cases

humitos · 2017-11-30T23:39:08Z

readthedocs_build/config/test_config.py

+            source_file=str(tmpdir.join('readthedocs.yml')),
+            source_position=0)
+        build.validate_build()
+        assert build['build']['image'] == '2.0'


Same here but 2.7, I guess.

Not sure we want to specify python magically, based on container image, do we?

We already do this to a certain degree, but i think the python version tests are testing some of this. For example:

python['version'] = 3 is valid, and exists in the dashboard, we should select the right python 3 version (pretty sure tests exist here)

python['version'] = None should return 2.7 for now, until we're opinionated about forcing 3.6 default (not work i think we need to make for ourselves just yet, but i'd like to see this soon)

If we don't want that, what would it happen if the user doesn't specify the python version? should the test fail? do we have a test for that?

agjohnson

Looks good. I think some of the changes here are regressing on configuration ability and our error reporting ability, but it seems we can uphold these with a bit more logic around validation and ordering.

agjohnson · 2017-12-01T02:01:50Z

docs/spec.rst

+  Options for setting the docker configuration.
+
+    ``image``
+    This sets the docker image to use on the build, as defined `here <https://github.com/rtfd/readthedocs-docker-images/blob/master/CONTRIBUTING.rst#releases>`_.


perhaps "docker image" -> "build image" for consistency on nomenclature. It's not a docker image to end users, nor can arbitrary docker images be used.

agjohnson · 2017-12-01T02:02:36Z

readthedocs_build/config/config.py

@@ -30,6 +26,18 @@
 TYPE_REQUIRED = 'type-required'
 PYTHON_INVALID = 'python-invalid'

+DOCKER_BUILD_IMAGES = {


+1 on a setting, we shouldn't change that behavior. This is important for all non-prod instances.

agjohnson · 2017-12-01T02:03:27Z

readthedocs_build/config/config.py

-        except (KeyError, TypeError):
-            pass
-        return self.PYTHON_SUPPORTED_VERSIONS
-


What's the reason to remove this? Seems we want validation on this still.

agjohnson · 2017-12-01T02:05:26Z

readthedocs_build/config/config.py

+
+        # Set the valid python versions for this container
+        img = build['image']
+        self.PYTHON_SUPPORTED_VERSIONS = DOCKER_BUILD_IMAGES[img]['python']['supported_versions']


PYTHON_SUPPORTED_VERSIONS is named as a constant and shouldn't be treated as a variable. I think the usage of get_valid_python_versions was more correct. Also, validation steps probably shouldn't have side effects like altering the state here, so that's probably also another good reason to make this a method that merges sources together.

The ordering of the validation is going to continue to matter (because we need to read the docker image, then determine python version), so it will have to set state somewhere, but agreed on making it a bit nicer.

I believe we are/were doing ordering of validation one level up anyways:
https://github.com/rtfd/readthedocs-build/blob/master/readthedocs_build/config/config.py#L133-L157

So, dependencies would naturally be filled there, as the validation steps are ordered there

agjohnson · 2017-12-01T02:06:35Z

readthedocs_build/config/test_config.py

-            env_config={'python': {'supported_versions': [3.5, 3.6]}}
-        )
-        build.validate_python()
-        assert build['python']['version'] == 3.6


Seems we should make this error work instead of removing it. We use supported versions for giving intelligent errors to users.

agjohnson · 2017-12-01T02:16:07Z

readthedocs_build/config/test_config.py

+            source_file=str(tmpdir.join('readthedocs.yml')),
+            source_position=0)
+        build.validate_build()
+        assert build['build']['image'] == 'latest'


I don't think we're opinionated about this yet. We should be eventually though.

+1 on checks for default version though, that seems to be missing from test cases

agjohnson · 2017-12-01T02:22:55Z

readthedocs_build/config/test_config.py

+            source_file=str(tmpdir.join('readthedocs.yml')),
+            source_position=0)
+        build.validate_build()
+        assert build['build']['image'] == '2.0'


We already do this to a certain degree, but i think the python version tests are testing some of this. For example:

python['version'] = 3 is valid, and exists in the dashboard, we should select the right python 3 version (pretty sure tests exist here)

python['version'] = None should return 2.7 for now, until we're opinionated about forcing 3.6 default (not work i think we need to make for ourselves just yet, but i'd like to see this soon)

humitos

Just small suggestions/comments.

humitos · 2017-12-07T17:49:41Z

readthedocs_build/config/config.py

+# These map to coordisponding settings in the .org,
+# so they haven't been renamed.
+DOCKER_IMAGE = '{}:{}'.format(DOCKER_DEFAULT_IMAGE, DOCKER_DEFAULT_VERSION)
+DOCKER_IMAGE_SETTINGS = {


shoulnd't we have the same .update() we have in .org?

https://github.com/rtfd/readthedocs.org/pull/3339/files#diff-3fbfb50cb375355069e83003f500d2c8R44

or at least, the getattr since otherwise there is no way to override this.

Yea, fixed that below -- I don't want this code to depend on RTD settings, so we pass it in.

humitos · 2017-12-07T18:13:06Z

readthedocs_build/config/config.py

+                        str(_build['image']),
+                        self.DOCKER_SUPPORTED_VERSIONS,
+                    )
+            if ':' not in build['image']:


it seems that this condition will be always True, since we are accepting 1.0, 2.0 and latest as valid choices (from DOCKER_SUPPORTED_VERSIONS)

If the build image is passed from RTD it will be fully qualified already.

Also the default (not from YAML)

Not sure if I follow you. If the image is passed from RTD or not from YAML we won't be inside if 'build' in self.raw_config since I understood that raw_config is the YAML. So, maybe this if should be outside this block.

humitos · 2017-12-07T23:07:20Z

readthedocs_build/config/test_config.py

+            source_position=0)
+        with raises(InvalidConfig) as excinfo:
+            build.validate_build()
+        assert excinfo.value.key == 'build'


Just curious, why we don't use something like regular unittest.TestCase and assertEqual and those stuffs in this project?

That is just how this code was written originally. shrug

humitos · 2017-12-24T16:41:25Z

This PR closes #24

ericholscher added 3 commits November 30, 2017 11:49

Add the ability to add a docker config image

827182b

Support docker and python validation in the rtd build configs

2db1cec

Clean up line

9f17586

ericholscher mentioned this pull request Nov 30, 2017

Add ability to specify which version of Docker image to use readthedocs/readthedocs.org#3338

Closed

ericholscher added 2 commits November 30, 2017 13:38

pin pytest-describe

d766f41

Pin pytest

d93c81c

ericholscher requested review from agjohnson and humitos November 30, 2017 21:41

ericholscher mentioned this pull request Nov 30, 2017

Add docker image from the YAML config integration readthedocs/readthedocs.org#3339

Merged

humitos reviewed Nov 30, 2017

View reviewed changes

agjohnson reviewed Dec 1, 2017

View reviewed changes

ericholscher added 8 commits December 5, 2017 10:43

Small cleanup

4d8537d

Add back missing tests

3ae0685

Clean up logic

23cf764

More small bits

91d49ef

Cleanup linting

de3e359

More docstrings

b597d3b

A bit more clarification

73a184a

Fix logic

79f78d2

humitos reviewed Dec 7, 2017

View reviewed changes

Support passing full setting through

8e8164e

humitos mentioned this pull request Dec 17, 2017

Support for UML via PlantUML & sphinxcontrib-plantuml readthedocs/readthedocs.org#407

Closed

humitos mentioned this pull request Dec 28, 2017

Check versions used to create the venv and auto-wipe readthedocs/readthedocs.org#3432

Merged

ericholscher merged commit c144e43 into master Dec 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ability to add a docker config image #33

Add the ability to add a docker config image #33

ericholscher commented Nov 30, 2017 •

edited

Loading

humitos Nov 30, 2017

ericholscher Dec 1, 2017

agjohnson Dec 1, 2017

humitos Nov 30, 2017

agjohnson Dec 1, 2017

humitos Nov 30, 2017

ericholscher Dec 1, 2017

agjohnson Dec 1, 2017

humitos Dec 1, 2017

agjohnson left a comment

agjohnson Dec 1, 2017

agjohnson Dec 1, 2017

agjohnson Dec 1, 2017

agjohnson Dec 1, 2017

ericholscher Dec 1, 2017

agjohnson Dec 1, 2017

agjohnson Dec 1, 2017

agjohnson Dec 1, 2017

agjohnson Dec 1, 2017

humitos left a comment

humitos Dec 7, 2017

ericholscher Dec 8, 2017

humitos Dec 7, 2017

ericholscher Dec 8, 2017

ericholscher Dec 8, 2017

humitos Dec 11, 2017

humitos Dec 7, 2017

ericholscher Dec 8, 2017

humitos commented Dec 24, 2017

Add the ability to add a docker config image #33

Add the ability to add a docker config image #33

Conversation

ericholscher commented Nov 30, 2017 • 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

agjohnson left a 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 left a 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 Dec 24, 2017

ericholscher commented Nov 30, 2017 •

edited

Loading