High-level getter/setter for the YAML tree used for transferring comments #32
Conversation
Signed-off-by: Dennis Marttinen <dennis@weave.works>
|
|
||
| // Cast the object to a metav1.Object to get access to annotations. | ||
| // If this fails, the given object does not support storing comments. | ||
| metaObj, ok := toMetaObject(obj) |
There was a problem hiding this comment.
For an other refactor, but we shall make a union interface between metav1.Object and runtime.Object in order to have better type-safety for these kinds of things.
| // SetCommentSource sets the given YAML tree as the source for transferring comments for the given runtime.Object. | ||
| // This may be used externally to implement e.g. re-parenting of the comment source tree when moving structs around. | ||
| func SetCommentSource(obj runtime.Object, source *yaml.RNode) error { | ||
| // Convert the given tree into a string. This also handles the source == nil case. |
There was a problem hiding this comment.
So the String function is set on the pointer to RNode and handles itself being nil?
|
|
||
| // GetCommentSource retrieves the YAML tree used as the source for transferring comments for the given runtime.Object. | ||
| // This may be used externally to implement e.g. re-parenting of the comment source tree when moving structs around. | ||
| func GetCommentSource(obj runtime.Object) (*yaml.RNode, error) { |
There was a problem hiding this comment.
Please create some simple unit tests for these functions, and use them in the rest of the code internally
Signed-off-by: Dennis Marttinen <dennis@weave.works>
twelho
force-pushed
the
comment-source-access
branch
from
0be300f to
c69d0f6
Compare
Signed-off-by: Dennis Marttinen <dennis@weave.works>
Signed-off-by: Dennis Marttinen <dennis@weave.works>
twelho
force-pushed
the
comment-source-access
branch
from
ec903ee to
ddf485a
Compare
Signed-off-by: Dennis Marttinen <dennis@weave.works>
pkg/serializer/comments.go
Outdated
| } | ||
|
|
||
| // SetCommentSource sets the given bytes as the source for transferring comments for the given runtime.Object. | ||
| func setCommentSourceBytes(obj runtime.Object, source []byte) error { |
There was a problem hiding this comment.
as this expects to return either errNoObjectMeta or nil, please use just an ok bool return value so the panic above is unnecessary
pkg/serializer/comments.go
Outdated
| if !ok { | ||
| // no need to delete the annotation as we know it doesn't exist, just do a normal encode | ||
| priorNode, err := getCommentSourceMeta(metaObj) | ||
| if err == ErrNoStoredComments { |
There was a problem hiding this comment.
use errors.Is here instead, to allow future wrapping of the error
pkg/serializer/comments.go
Outdated
| if !ok { | ||
| // If the object doesn't have ObjectMeta embedded, just do nothing | ||
| // Preserve the original file content in the annotation (this requires embedding ObjectMeta). | ||
| if err := setCommentSourceBytes(obj, doc); err == ErrNoObjectMeta { |
There was a problem hiding this comment.
this should use just an if !ok { do things } to avoid panicing
| @@ -14,6 +15,11 @@ import ( | |||
|
|
|||
| const preserveCommentsAnnotation = "serializer.libgitops.weave.works/original-data" | |||
|
|
|||
| var ( | |||
| ErrNoObjectMeta = errors.New("the given object cannot store comments, it is not metav1.ObjectMeta compliant") | |||
There was a problem hiding this comment.
Could you add a TODO here for the future to investigate if we can make this just require metav1.Object (the interface) instead of ObjectMeta the struct.
Also please add comments to both of these errors
pkg/serializer/comments_test.go
Outdated
| t.Errorf("expected error %t, but received %t: %v", tc.expectedErr, actualErr != nil, actualErr) | ||
| } | ||
|
|
||
| if actualErr == nil { |
There was a problem hiding this comment.
I prefer if actualErr != nil { return // short circuit } pretty much everywhere
Signed-off-by: Dennis Marttinen <dennis@weave.works>
twelho
force-pushed
the
comment-source-access
branch
from
645e711 to
1a25766
Compare
|
Comments addressed and CI is green, merging... |
Implements high-level
GetCommentSourceandSetCommentSourcefunctions for accessing the*yaml.RNodetree used internally for transferring comments. This access enables applications pulling inlibgitopsto do advanced operations while still preserving comment transfer abilities, they can e.g. re-parent the comment source tree (move it under a sub-node) to match changes in the object's structure.cc @luxas, feedback would be appreciated on whether you think these are sensible/in the right place/integrated well enough with the rest of the comment handling.