-
Notifications
You must be signed in to change notification settings - Fork 20
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
merge stable to release 1.1 #5300
Conversation
Number attributes do not accept boolean values
* add failing test * fix overlapping partial diffs issue
@@ -668,7 +668,7 @@ async def query_init(self, db: InfrahubDatabase, **kwargs: Any) -> None: | |||
AND (from_time <= diff_rel.from < $to_time) | |||
AND (diff_rel.to IS NULL OR (from_time <= diff_rel.to < $to_time)) | |||
AND r_root.from <= diff_rel.from | |||
AND (r_root.to IS NULL OR r_root.to >= diff_rel.from) | |||
AND (r_root.to IS NULL OR diff_rel.branch <> r_root.branch OR r_root.to >= diff_rel.from) |
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 little change is required to account for this case
- make branch
branch
- delete
NodeA
onmain
- update a relationship to
NodeA
onbranch
, whereNodeA
is not deleted
if self.cardinality is RelationshipCardinality.ONE: | ||
actions = {sr.action for sr in single_relationships if sr.action is not DiffAction.UNCHANGED} | ||
if len(actions) == 1: | ||
action = actions.pop() |
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.
ignore UNCHANGED
individual relationships when determining the overall action for this relationship group (kind of equivalent to RelationshipManager)
delete_branch = diff_branch | ||
update_branch = default_branch | ||
else: | ||
delete_branch = default_branch | ||
update_branch = diff_branch |
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 naming and logic was inverted here, which confused me while investigating this test
CodSpeed Performance ReportMerging #5300 will not alter performanceComparing Summary
|
diff_relationship = DiffRelationshipIntermediate( | ||
name=relationship_schema.name, | ||
cardinality=relationship_schema.cardinality, | ||
identifier=relationship_schema.get_identifier(), | ||
from_time=from_time, |
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 use the from_time
here and above in _get_diff_attribute
when determining the action (ADDED/UPDATED/REMOVED) for a given database path. when we are getting nodes on the base branch (always main
for now), we retrieve both completely new nodes that are not yet part of this diff and updated nodes that are already part of this diff. we need to use different from_time
s when determining their actions and this is how we accomplish that
def get_current_node_field_specifiers(self) -> set[NodeFieldSpecifier]: | ||
new_node_field_specifiers = self.get_new_node_field_specifiers() | ||
current_node_field_specifiers = self._previous_node_field_specifiers - new_node_field_specifiers | ||
return current_node_field_specifiers |
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'd like to add some caching for these b/c we should really only need to calculate these things one time, but that can come in a later PR
had to make a separate branch to address how some diff query changes on
stable
interact with other diff changes onrelease-1.1
had to make more changes than expected to the diff calculation logic to interact correctly with a bug that I fixed in
stable
. fortunately, we have a lot of tests that cover the diff pretty well