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
Merged

Conversation

@humitos
Copy link
Member

@humitos humitos commented Jan 22, 2019

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
@humitos humitos requested review from stsewd and Jan 22, 2019
@humitos humitos force-pushed the humitos/latest-docker-images branch from c18bf2b to 9041e4a Jan 22, 2019
@@ -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
Copy link
Member

@stsewd stsewd Jan 22, 2019

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

Copy link
Member Author

@humitos humitos Jan 23, 2019

🤔 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

@stsewd stsewd Jan 23, 2019

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

@humitos humitos Jan 23, 2019

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

@agjohnson agjohnson Jan 23, 2019

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

@agjohnson agjohnson Jan 23, 2019

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

@humitos humitos Jan 23, 2019

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

@humitos humitos Jan 23, 2019

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.

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

@stsewd stsewd Jan 22, 2019

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

Copy link
Member

@stsewd stsewd Jan 22, 2019

Means we can still override them from the admin

Copy link
Member Author

@humitos humitos Jan 23, 2019

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

Copy link
Member Author

@humitos humitos Jan 23, 2019

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

Copy link
Member

@stsewd stsewd Jan 23, 2019

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.

Copy link
Member

@stsewd stsewd Jan 23, 2019

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

@humitos
Copy link
Member Author

@humitos humitos commented Jan 23, 2019

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

@@ -146,6 +147,14 @@ class BuildConfigBase:
'mkdocs',
'submodules',
]

valid_build_images = {'stable', 'latest'}
for k in DOCKER_IMAGE_SETTINGS:
Copy link
Member

@stsewd stsewd Jan 23, 2019

We should put this in a function or property

@humitos humitos force-pushed the humitos/latest-docker-images branch from 5749259 to 320ea9f Jan 23, 2019
stsewd
stsewd approved these changes Jan 24, 2019
Copy link
Member

@stsewd stsewd left a comment

CI failure is only because of the linter

@agjohnson agjohnson removed this from the 2.9 milestone Jan 25, 2019
@agjohnson agjohnson added this to the 3.2 milestone Jan 25, 2019
humitos added 11 commits Jan 25, 2019
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
Copy link
Member Author

@humitos humitos commented Jan 25, 2019

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.

@humitos humitos force-pushed the humitos/latest-docker-images branch from 8126b74 to a7770c0 Jan 25, 2019
Copy link
Member

@ericholscher ericholscher left a comment

Looks good. Let's ship it :)

@ericholscher ericholscher merged commit 04fd0c2 into master Jan 29, 2019
1 of 2 checks passed
@delete-merged-branch delete-merged-branch bot deleted the humitos/latest-docker-images branch Jan 29, 2019
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

4 participants