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

Feature flag to use readthedocs/build:testing image #5109

Merged
merged 2 commits into from Jan 15, 2019

Conversation

@humitos
Copy link
Member

@humitos humitos commented Jan 15, 2019

This is a new simple feature flag to allow us to put some projects under readthedocs/build:testing image so we can start testing 5.0rc1 in some sampled projects.

I decided to use :testing so now we can tag 5.0rc1 as testing

docker tag readthedocs/build:5.0rc1 readthedocs/build:testing

and later (in months) we can re-tag it to 6.0rc1 for example just by running another docker command in the builders without modifying Python code.

I'd like to merge this soon and deploy it so we can start our new release process.

Closes #5106

@humitos humitos requested a review from Jan 15, 2019
@humitos humitos self-assigned this Jan 15, 2019
Copy link
Member

@ericholscher ericholscher left a comment

👍 on the concept, I do think we should respect the users config if set explicitly before our feature flag tho.

if self.project.container_image:
self.container_image = self.project.container_image
# ``testing`` image if the project has this feature flag
if self.project.has_feature(Feature.USE_TESTING_BUILD_IMAGE):
self.container_image = 'readthedocs/build:testing'
Copy link
Member

@ericholscher ericholscher Jan 15, 2019

Shouldn't this be first so that it doesn't overwrite the users config?

Copy link
Member

@stsewd stsewd Jan 15, 2019

I think the idea was to override the users config

Copy link
Member

@ericholscher ericholscher Jan 15, 2019

I think if we're trying to test things, we should do it on users that haven't explicitly expressed a preference. It feels wrong to override them, and more likely to break something.

Copy link
Member Author

@humitos humitos Jan 15, 2019

My idea is to override this user setting, yes. I explained my opinion better at #5109 (comment)

if self.config and self.config.build.image:
self.container_image = self.config.build.image
# the image overridden by the project (manually set by an admin) or,
if self.project.container_image:
self.container_image = self.project.container_image
Copy link
Member

@ericholscher ericholscher Jan 15, 2019

I also wonder if this should be before the users config? I can see reasons for both, but having our DB entry overwrite the users wishes seems really confusing.

Copy link
Member

@ericholscher ericholscher Jan 15, 2019

Hrm ok. I guess the DB setting makes a bit of sense to overwrite, but not a feature flag for testing.

@stsewd
Copy link
Member

@stsewd stsewd commented Jan 15, 2019

We could add the feature flag here, in the defaults of the config module

https://github.com/rtfd/readthedocs.org/blob/7aa57bbcd01994b662f34e3a7bf7ea8cb7ab82ac/readthedocs/doc_builder/config.py#L55

There we always have priority over the user's config

@humitos
Copy link
Member Author

@humitos humitos commented Jan 15, 2019

I do think we should respect the users config if set explicitly before our feature flag tho.

Usually, users can't override what we set from the admin for an internal purpose (container_image for example) or any other Feature flag (I see feature flags as "a way to force something to happen" or "enable/disable something that otherwise is not possible").

Example: if the user is using 2.0 as an image, we won't be able to change it without depending on the user interaction. In fact, the user won't be able to do it because readthedocs/build:testing is not possible to define in the YAML. So we will need 1) activate the flag in the project and 2) communicate this to the user so she/he removes the hardcoded build.image value from the YAML file.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jan 15, 2019

So we will need 1) activate the flag in the project and 2) communicate this to the user so she/he removes the hardcoded build.image value from the YAML file.

Well the idea of this flag is to have it be a "mass testing" flag, so not something that we'd do when a user requests it. The goal was to decouple user requests from our own beta testing, so it feels like we should only be doing mass testing on projects that haven't explicitly defined a container, otherwise we'd be overriding user intent with our own beta testing, which feels wrong.

1. `testing` if the project has the feature flag
2. defined by the user in YAML file
3. set by `project.container_image` if an admin override it

The image used will be the last one that it's true following that order.
@humitos
Copy link
Member Author

@humitos humitos commented Jan 15, 2019

OK, @ericholscher, I followed what you said and made the changes.

@humitos humitos merged commit cbdf5b5 into master Jan 15, 2019
1 of 2 checks passed
@delete-merged-branch delete-merged-branch bot deleted the humitos/feature-flag-testing-image branch Jan 15, 2019
@stsewd
Copy link
Member

@stsewd stsewd commented Jan 15, 2019

We should make this change in the defaults that we pass to the config module

#5109 (comment)

otherwise this isn't going to be saved in the build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants