feat: modifying error message for subobject #607
Conversation
… once, so iterate test case seems not working here?
| assert.Equal(t, err.Error(), expected) | ||
| } | ||
|
|
||
| func TestWrongKRM(t *testing.T) { |
There was a problem hiding this comment.
Write tests separately instead of using parameterized way because recover panic can only catch error once. Not sure if there is a better way to include all test cases about error messaging testing into one Test function.
|
Can you change the PR subject to be more specific and add a PR description to explain what this PR does and the problems it tries to solve? |
go/fn/object.go
Outdated
| return &SubObject{obj: internal.NewMap(variant.YNode())} | ||
| // this SubObject is the root kubeObject | ||
| if o.nodePath == nil { | ||
| oGroup, oVersion := ParseGroupVersion(o.GetString("apiVersion")) |
There was a problem hiding this comment.
As we discussed offline. o is not a KubeObject, so we should not call the KRM specific functions on it (apiVersion , kind, namespace)
There was a problem hiding this comment.
GetMap function can be called by KubeObject or SubObject. Here I just separating the condition when the SubObject is KubeObject. I see your point and I agree that for SubObject there should not have stuff like GVK, but I am not sure about what structure to design to fix it
There was a problem hiding this comment.
I agree that it is a bit odd for a SubObject-specific function to be having references to GVK. I see that below you are using o.gvk, is there a reason we can't use that here? Is it empty when o is a KubeObject? I'm wondering why we need this extra case, because I find it surprising if o.gvk is not the object's GVK.
There was a problem hiding this comment.
When nodepath == nil, that is the top level of the resource. In this case, SubObject equals its parent KubeObject. Only in this case gvk exists, and it would be assigned to subObject to inherit. All the SubObject below this level would inherit the same gvk from this top-level gvk
There was a problem hiding this comment.
Ah, I see what you are doing here. Basically this logic is based on the assumption that users will always call GetMap to start using SubObject, which kind of works in the current code base. But it potentially can be a bug prone solution. What if they instantiate their own SubObject and never call GetMap to initialize the gvk (internal)? what if they instantiate an empty SubObject and call this code (panic 😱 )?
There was a problem hiding this comment.
Yeah, I think in the ideal case, we would have the following:
o.gvkgets populated only when the KubeObject is created. I don't think theSubObjectcan ever make the assumption that it belongs to a KubeObject because the relationship is the other way around. The KubeObject always contains a SubObject, but it may not always be the case that a particular SubObject belongs to a KubeObject in the case that the user initializes their own SubObject. So GVK may not always make sense in a SubObject.- The SubObject code can't assume we have
o.gvkin that case, because the SubObject may not have been created from a KubeObject.
Is it possible to populate o.gvk only on creation of the KubeObject (point 1 above) and still get the nicer error message that you want? If we do that, then we can just check if o.gvk is empty or not before we use it, to satisfy point 2.
|
I suggest pausing this PR for now and focus on the |
zyy98
changed the title
3 participants
Current subobject does not give information about where the error was. When a file contains multiple resources, it is difficult to see where the error is. By adding GVK and relative path to KubeObject, it would be much easier to see what went wrong