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

Conversation

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Nov 30, 2017

This depends on readthedocs/readthedocs-build#33

Fixes #3338

@ericholscher ericholscher force-pushed the add-docker-image-option branch from 7d45b6a to aa15e78 Nov 30, 2017
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

@humitos humitos Nov 30, 2017

How does this assert work?

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

Copy link
Contributor

@agjohnson agjohnson Dec 1, 2017

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

Copy link
Member Author

@ericholscher ericholscher Dec 5, 2017

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

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

@humitos humitos Nov 30, 2017

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

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

@humitos humitos Nov 30, 2017

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

@agjohnson agjohnson Dec 1, 2017

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

Copy link
Contributor

@agjohnson agjohnson left a comment

Definitely a good step to a more complete config. Similar to the readthedocs-build change, there are a few points of regression in configuration and user reporting that i think this needs to address, but this mostly looks to be due to moved logic.


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

@agjohnson agjohnson Dec 1, 2017

For consistency and clarity:

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

if they want to be on the cutting edge.

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

@agjohnson agjohnson Dec 1, 2017

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

@agjohnson agjohnson Dec 1, 2017

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

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

@agjohnson agjohnson Dec 1, 2017

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

'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

@agjohnson agjohnson Dec 1, 2017

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

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

@agjohnson agjohnson Dec 1, 2017

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

```````````

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

@agjohnson agjohnson Dec 1, 2017

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

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
Contributor

@agjohnson agjohnson Dec 1, 2017

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

}),
])
self.assertEqual(config.python_version, 2)

Copy link
Contributor

@agjohnson agjohnson Dec 1, 2017

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.

@ericholscher
Copy link
Member Author

@ericholscher ericholscher commented Dec 5, 2017

@agjohnson @humitos these should be ready for another review.

@@ -133,8 +133,24 @@ def load_yaml_config(version):
parsing consistent between projects.
"""
checkout_path = version.project.checkout_path(version.slug)
env_config = {}
Copy link
Member

@humitos humitos Dec 5, 2017

This one is not needed. You are re-creating it some lines below.

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Dec 11, 2017

Before this can go out, we need to address #3190. Changing your docker image will break your builds at the moment, as the virtualenv has python 2.7 from the 2.0 image, and when we all of a sudden use the python 2.7 from latest, parts of the virtualenv break.

old_config = getattr(settings, 'DOCKER_BUILD_IMAGES', None)
if old_config:
log.warning('Old config detected, DOCKER_BUILD_IMAGES->DOCKER_IMAGE_SETTINGS')
DOCKER_IMAGE_SETTINGS.update(old_config)
Copy link
Contributor

@agjohnson agjohnson Dec 11, 2017

This warning could be a good use of a django system check warning as well.

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Dec 11, 2017

Looks good, we can discuss best way to wipe venv, or best way to detect when to wipe the venv.

Reminder before merge here, requirements needs updating.

@ericholscher ericholscher merged commit da7ec2b into master Dec 28, 2017
2 checks passed
achimnol added a commit to achimnol/aiotools that referenced this issue Jan 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants