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 future default true to Feature flags #7524

Merged
merged 8 commits into from Oct 19, 2020
Merged

Conversation

ericholscher
Copy link
Member

This also renames default_true to past_default_true,
making it more explicit as to which does which.

This will give us a lot more flexibility with feature flags,
since we often want to have one of these options with them.

@ericholscher ericholscher requested a review from humitos Oct 1, 2020
name='environmentvariable',
options={'get_latest_by': 'modified', 'ordering': ('-modified', '-created')},
),
migrations.RemoveField(
Copy link
Member Author

Choose a reason for hiding this comment

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

This didn't rename the field, will need to fix that :(

@ericholscher ericholscher requested review from a team and removed request for humitos Oct 1, 2020
@ericholscher ericholscher added the PR: work in progress Pull request is not ready for full review label Oct 1, 2020
@ericholscher ericholscher removed the request for review from a team Oct 1, 2020
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

I think this is super handy!

I know that @stsewd were doing multiple migrations when deleting fields to avoid breaking on deploy. We may need to do the same here since we are renaming a field, so old instances will try to hit the old name.

migrations.AlterModelOptions(
name='environmentvariable',
options={'get_latest_by': 'modified', 'ordering': ('-modified', '-created')},
),
Copy link
Member

Choose a reason for hiding this comment

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

This looks unrelated here. I don't know where this came from... 😕

@stsewd
Copy link
Member

stsewd commented Oct 5, 2020

yeah, this is going to break during deploy. We should do something like:

During a first deploy

  • create the new fields, don't delete the old one (make it nullable here)
  • Run a data migration from the data of the old field to the new field (we shouldn't create new feature flags with default=true during this time, or make the data migration during the second deploy)

During a second deploy

  • Replace the usage of the old field with the new field
  • Delete the old field

@humitos
Copy link
Member

humitos commented Oct 12, 2020

Once this gets merged and deployed, I'd like to mark future_default_true=True to the following feature flags and update its add_date accordingly:

  • LIMIT_CONCURRENT_BUILDS
  • DEDUPLICATE_BUILDS
  • SPHINX_PARALLEL (edit): we can't enable this yet because we are not installing latest readthedocs-sphinx-extension by default and it's not parallel safe, which makes everybody using fail_on_warning=True to fail the build.
  • VCS_REMOTE_LISTING

BTW, using the add_date (creation date) to compare which projects are going to be affected by the feature flag may end up with "invalid" datetime if it's updated. We are using the same datetime for two different things: "creation date of the object" and "date the feature start taking effect". If we want to keep those different dates, we should add a different field into the model.

This also renames `default_true` to `past_default_true`,
making it more explicit as to which does which.

This will give us a lot more flexibility with feature flags,
since we often want to have one of these options with them.
@ericholscher
Copy link
Member Author

ericholscher commented Oct 16, 2020

Alright, I updated this PR to not change the existing model. I think it's fine for now, and we can adjust in the future if we need to.

@ericholscher
Copy link
Member Author

ericholscher commented Oct 16, 2020

BTW, using the add_date (creation date) to compare which projects are going to be affected by the feature flag may end up with "invalid" datetime if it's updated. We are using the same datetime for two different things: "creation date of the object" and "date the feature start taking effect". If we want to keep those different dates, we should add a different field into the model.

Agreed. We should probably add a date field and use that, but I think the created date will never be automatically updated, so I don't think it's a huge issue.

@ericholscher ericholscher requested a review from a team Oct 16, 2020
readthedocs/projects/models.py Show resolved Hide resolved
@humitos humitos removed the PR: work in progress Pull request is not ready for full review label Oct 19, 2020
stsewd
stsewd approved these changes Oct 19, 2020
Co-authored-by: Manuel Kaufmann <humitos@gmail.com>
@ericholscher ericholscher merged commit 1259825 into master Oct 19, 2020
2 checks passed
@ericholscher ericholscher deleted the future-default-true branch Oct 19, 2020
@humitos
Copy link
Member

humitos commented Oct 20, 2020

We should probably add a date field and use that, but I think the created date will never be automatically updated, so I don't think it's a huge issue.

It's not because it will be updated automatically, just that when we changed it manually we loose the original date that feature was created. For example, after today's deploy I want every new project to use a feature that already existed, so I will change the created date and loose the original one.

@ericholscher
Copy link
Member Author

Ah, I see what you mean. Do we care about the actual created date on the model?

@humitos
Copy link
Member

humitos commented Oct 20, 2020

I don't think it's important, but it's a nice to have to me.

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