-
Notifications
You must be signed in to change notification settings - Fork 405
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
feat: Create versioning for segments #4138
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Uffizzi Preview |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4138 +/- ##
==========================================
+ Coverage 96.57% 96.60% +0.02%
==========================================
Files 1191 1194 +3
Lines 38809 39071 +262
==========================================
+ Hits 37481 37745 +264
+ Misses 1328 1326 -2 ☔ View full report in Codecov by Sentry. |
api/segments/migrations/sql/0023_add_versioning_to_segments.sql
Outdated
Show resolved
Hide resolved
api/segments/models.py
Outdated
new_segment = Segment.objects.create( | ||
name=self.name, | ||
description=self.description, | ||
project=self.project, | ||
feature=self.feature, | ||
version=self.version, | ||
version_of=self, | ||
) |
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.
We have clone functions in other models in the code base where we take a slightly different approach, e.g.:
def deep_clone(self) -> "Segment":
new_segment = copy.deepcopy(self)
new_segment.id = None # this is what forces django to think it's creating a new model object
...
The benefit of this approach is that it maintains itself when new fields are added to those model classes. With the approach that you're using here, if a new field is added to the segment, we'll need to remember to add it here I think?
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.
Ok I'll implement that strategy. Just to double check, the timestamps will still be set to now
for the new model, right?
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.
Ok this is done.
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.
Just to double check, the timestamps will still be set to now for the new model, right?
Good question - I don't know. We should verify in a test. Although I guess the question is whether we want that to be the case. Since we're backfilling the history, wouldn't we want e.g. the created_at
not to change?
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.
Well it turns out that none of the models involved in this PR have created_at
or updated_at
timestamps, although they probably should, at least on the segments. Should I add them to this PR? If so, what should the default be for existing objects None
or now()
? Should I include conditions and rules in having timestamps as well?
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.
Let's add these fields to all models in the segments app, yes, but let's do it in a separate PR. Regarding the default, we could be smart and look in the objects history but that might end up being a fairly mammoth migration.
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.
Ok sounds good I've made a new ticket for this.
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.
The PR is up for the timestamps:
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've added a few more comments but I also noticed that none of the view
tests actually verify that the clone is created (perhaps because they rely on the number of audit records instead, which I'm suggesting that we remove).
api/segments/models.py
Outdated
new_segment = Segment.objects.create( | ||
name=self.name, | ||
description=self.description, | ||
project=self.project, | ||
feature=self.feature, | ||
version=self.version, | ||
version_of=self, | ||
) |
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.
Just to double check, the timestamps will still be set to now for the new model, right?
Good question - I don't know. We should verify in a test. Although I guess the question is whether we want that to be the case. Since we're backfilling the history, wouldn't we want e.g. the created_at
not to change?
Thanks for submitting a PR! Please check the boxes below:
pre-commit
to check lintingdocs/
if required so people know about the feature!Changes
This introduces versioning for segments by cloning their features into new objects and filtering out older versions of segment through a new manager class.
How did you test this code?
A few new unit tests around the models as well as updates to some existing tests to handle audit log cases.