deepcopy for mutation parser.Node types #1121
Conversation
brycecr
changed the title
Codecov Report
@@ Coverage Diff @@
## master #1121 +/- ##
==========================================
+ Coverage 47.84% 48.33% +0.48%
==========================================
Files 62 62
Lines 4264 4283 +19
==========================================
+ Hits 2040 2070 +30
+ Misses 1966 1959 -7
+ Partials 258 254 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
pkg/mutation/path/parser/node.go
Outdated
| } | ||
| for i := 0; i < len(r.Nodes); i++ { | ||
| var c Node | ||
| switch n := r.Nodes[i].(type) { |
There was a problem hiding this comment.
We could have the signature for DeepCopy() be:
DeepCopy() Node
and add it to the interface, that would eliminate the need for the switch statement.
This is similar to what K8s does for objects, though I think it's a bit different than what we're doing for other parts of mutation.
Curious to hear others' thoughts on returning an interface vs. an object from DeepCopy
There was a problem hiding this comment.
Yeah I'd be curious what others thoughts are as well as I was trying to decide what to do here. I think if you add DeepCopy() Node to the interface, it is not fulfilled by DeepCopy() Path, for instance, so one has to actually specify Node as the return type.
That means that uses of DeepCopy that want to assign to specific types have to assert/cast (this describes the current use in assignMetadata), but it also means that you don't need type-switch statements if you're copying Nodes without knowing their concrete type.
I went this way after some cursory looking through client-go, where it seems like DeepCopy is defined on all of the concrete types...but it's possible this isn't the way to go or that I read that code a bit wrong :)
There was a problem hiding this comment.
Ah, you're totally right. K8s uses DeepCopyObject when it expects a return type if the runtime.Object interface.
There was a problem hiding this comment.
So by that analog we could add a
func (n TypeGoesHere) DeepCopyNode() Node {
return n.DeepCopy()
}
to avoid the type switch and provide both options?
There was a problem hiding this comment.
I like that, like having a cake and DeepCopying it so you can eat it too
See new patch.
brycecr
force-pushed
the
brycecr-deepcopy
branch
from
6c4411f
to
dfd6280
Compare
Signed-off-by: Bryce Cronkite-Ratcliff <brycecr@gmail.com>
Signed-off-by: Bryce Cronkite-Ratcliff <brycecr@gmail.com>
Signed-off-by: Bryce Cronkite-Ratcliff <brycecr@gmail.com>
brycecr
force-pushed
the
brycecr-deepcopy
branch
from
dfd6280
to
68142eb
Compare
|
for some reason the mutation e2e tests are breaking now |
Signed-off-by: Bryce Cronkite-Ratcliff <brycecr@gmail.com>
|
Good find! |
What this PR does / why we need it:
Add DeepCopy Methods to Node-implementing types List, Path, and Object. Per #933, some pointers on these objects might not be properly copied in AssignMetadata.DeepCopy (where a slice of nodes was copied with
copy); in general, these seem like useful methods to have.Which issue(s) this PR fixes:
Fixes #993