-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Add future default true to Feature flags #7524
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
Conversation
name='environmentvariable', | ||
options={'get_latest_by': 'modified', 'ordering': ('-modified', '-created')}, | ||
), | ||
migrations.RemoveField( |
There was a problem hiding this comment.
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 :(
There was a problem hiding this 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')}, | ||
), |
There was a problem hiding this comment.
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... 😕
yeah, this is going to break during deploy. We should do something like: During a first deploy
During a second deploy
|
Once this gets merged and deployed, I'd like to mark
BTW, using the |
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.
9e66122
to
0a56fb7
Compare
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. |
Agreed. We should probably add a date field and use that, but I think the |
Co-authored-by: Manuel Kaufmann <[email protected]>
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 |
Ah, I see what you mean. Do we care about the actual created date on the model? |
I don't think it's important, but it's a nice to have to me. |
This also renames
default_true
topast_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.