Feature flag to use readthedocs/build:testing image
#5109
Conversation
| 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' |
There was a problem hiding this comment.
Shouldn't this be first so that it doesn't overwrite the users config?
There was a problem hiding this comment.
I think the idea was to override the users config
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We already do this from the config module
There was a problem hiding this comment.
Hrm ok. I guess the DB setting makes a bit of sense to overwrite, but not a feature flag for testing.
|
We could add the feature flag here, in the defaults of the config module There we always have priority over the user's config |
Usually, users can't override what we set from the admin for an internal purpose ( Example: if the user is using |
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.
|
OK, @ericholscher, I followed what you said and made the changes. |
delete-merged-branch
bot
deleted the
humitos/feature-flag-testing-image
branch
|
We should make this change in the defaults that we pass to the config module otherwise this isn't going to be saved in the build. |
This is a new simple feature flag to allow us to put some projects under
readthedocs/build:testingimage so we can start testing5.0rc1in some sampled projects.I decided to use
:testingso now we can tag5.0rc1astestingand later (in months) we can re-tag it to
6.0rc1for 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