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 merge function for cluster snapshot #301

Merged
merged 6 commits into from
Oct 11, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions changelog/v0.21.1/add-merge-function.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
changelog:
- type: NEW_FEATURE
description: Adds a `Merge` function to Snapshot and ClusterSnapshot so that two snapshots can be merged.
issueLink: https://github.com/solo-io/skv2/issues/302
35 changes: 35 additions & 0 deletions pkg/resource/snapshot.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,25 @@ func (s Snapshot) Clone(selectors ...GVKSelectorFunc) Snapshot {
return clone
}

// Merges the Snapshot with a Snapshot passed in as an argument. The values
// in the passed in Snapshot will take precedence when there is an object mapped
// to the same gvk and name in both Snapshots.
func (s Snapshot) Merge(toMerge Snapshot) Snapshot {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment to the method about the what it does, and the merge semantics

merged := s.Clone()
for gvk, objectsMap := range toMerge {
if _, ok := merged[gvk]; ok {
for name, object := range objectsMap {
// If there is already an object specified here, the object from toMerge
// will replace it
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this conflict resolution approach seem fine?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but you also need to loop over all of the objects in toMerge and add them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate? That's what I'm doing here

Copy link
Member

Choose a reason for hiding this comment

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

Ignore me, I'm an idiot 😆

merged[gvk][name] = object
}
} else {
merged[gvk] = objectsMap
}
}
return merged
}

// ClusterSnapshot represents a set of snapshots partitioned by cluster
type ClusterSnapshot map[string]Snapshot

Expand Down Expand Up @@ -128,3 +147,19 @@ func (cs ClusterSnapshot) Clone(selectors ...GVKSelectorFunc) ClusterSnapshot {
}
return clone
}

// Merges the ClusterSnapshot with a ClusterSnapshot passed in as an argument.
// If a cluster exists in both ClusterSnapshots, then both Snapshots for the
// cluster is merged; with the passed in ClusterSnapshot's corresponding Snapshot
// taking precedence in case of conflicts.
func (cs ClusterSnapshot) Merge(toMerge ClusterSnapshot) ClusterSnapshot {
Copy link
Member

Choose a reason for hiding this comment

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

Comment here as well

merged := cs.Clone()
for cluster, snapshot := range toMerge {
if baseSnap, ok := merged[cluster]; ok {
merged[cluster] = baseSnap.Merge(snapshot)
} else {
merged[cluster] = snapshot
}
}
return merged
}
49 changes: 49 additions & 0 deletions pkg/resource/snapshot_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,55 @@ var _ = Describe("Snapshot", func() {
Expect(controllerutils.ObjectsEqual(copiedPaint, paint)).To(BeFalse())
})

It("will merge two snapshots properly", func() {
paint := &testv1.Paint{
ObjectMeta: metav1.ObjectMeta{Name: "paint", Namespace: "x"},
}
paintOverride := &testv1.Paint{
ObjectMeta: metav1.ObjectMeta{Name: "override", Namespace: "x"},
}
paint2 := &testv1.Paint{
ObjectMeta: metav1.ObjectMeta{Name: "paint2", Namespace: "y"},
}
name := types.NamespacedName{
Namespace: "x",
Name: "paint",
}
name2 := types.NamespacedName{
Namespace: "y",
Name: "paint2",
}
cluster1Name := "cluster1"
cluster2Name := "cluster2"
leftSnapshot := resource.ClusterSnapshot{
cluster1Name: resource.Snapshot{
testv1.PaintGVK: map[types.NamespacedName]resource.TypedObject{
name: paint,
name2: paint2,
},
},
}
rightSnapshot := resource.ClusterSnapshot{
cluster1Name: resource.Snapshot{
testv1.PaintGVK: map[types.NamespacedName]resource.TypedObject{
name: paintOverride,
},
},
cluster2Name: resource.Snapshot{
testv1.PaintGVK: map[types.NamespacedName]resource.TypedObject{
name: paint,
},
},
}

snap := leftSnapshot.Merge(rightSnapshot)
Expect(snap[cluster1Name][testv1.PaintGVK][name].GetName()).
To(Equal(paintOverride.Name))
Expect(snap[cluster1Name][testv1.PaintGVK][name2].GetName()).
To(Equal(paint2.Name))
Expect(snap[cluster2Name][testv1.PaintGVK][name].GetName()).
To(Equal(paint.Name))
})
})

})