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 a project-level configuration for PR builds #7090

Merged
merged 8 commits into from May 26, 2020

Conversation

ericholscher
Copy link
Member

@ericholscher ericholscher commented May 18, 2020

This allows users with the feature flag to enable/disable this feature.

@ericholscher ericholscher changed the title Fix PR builds being marked built Add a project-level configuration for PR builds May 18, 2020
@ericholscher ericholscher requested a review from May 18, 2020
This allows users with the feature flag to enable/disable this feature.
Copy link
Member

@humitos humitos left a comment

It seems we are not using external_builds_enabled to check if PR builder is configured.

I think we will need a migration that set external_builds_enabled=True on those projects that have the flag, otherwise those projects will show they don't have it enabled in the form.

Probably, the easiest way to do this is by removing the Feature flag completely from the code and just use external_builds_enabled to check if enabled or not.

So, you can enable this feature by sending us an `email <mailto:support@readthedocs.org>`__ including your project URL.
This feature is currently enabled for a subset of our projects while being rolled out.
You can check to see if your project has it enabled by looking at the :guilabel:`Admin > Advanced settings` and look for :guilabel:`Build pull requests for this project`.
We are rolling this feature out based on the projects age on Read the Docs,
Copy link
Member

@humitos humitos May 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
We are rolling this feature out based on the projects age on Read the Docs,
We are rolling this feature out based on the projects' age on Read the Docs,

I always confuse this, so maybe I'm wrong here :)

@ericholscher
Copy link
Member Author

ericholscher commented May 19, 2020

@humitos the goal is not to release this all at once, but slowly roll it out to more projects via the feature flag. I've updated the migration.

@ericholscher
Copy link
Member Author

ericholscher commented May 19, 2020

I guess we could ship this widely, and just scale it up as users enable it, but that worried me a little bit because it could swamp our servers, but maybe it's time?

Copy link
Member

@humitos humitos left a comment

I'm 👍 on this. Makes sense with the check of self.proect.external_builds_enabled. I like the pattern, so we can enable this widely now and make users to opt-in. Later, we can just remove the flag and 🙏 😝

pytest-custom-exit-code==0.3.0
pytest-django==3.6.0
pytest-django==3.8.0
Copy link
Member Author

@ericholscher ericholscher May 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I upgraded these because then it gave me proper failures on the migrations instead of blowing up :)

# TODO: Remove this after migrations
null=True,
Copy link
Member

@humitos humitos May 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed when using a default= value. I'd remove it in this PR instead of forcing us to create a new one later.

Copy link
Member Author

@ericholscher ericholscher May 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That only works on the new code though, when we run the migration and the old code is still running, it will blow up the DB.

@ericholscher ericholscher merged commit 97caf50 into master May 26, 2020
2 checks passed
@ericholscher ericholscher deleted the feature-flag-pr-builder branch May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants