Skip to content

feat: modifying error message for subobject #607

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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

feat: modifying error message for subobject #607

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bc1d6e5
draft for modifying error message
zyy98 Jul 20, 2022
b580325
minor bug
zyy98 Jul 22, 2022
4023a78
refactor test code, but defer function once catch error, can only run…
zyy98 Jul 22, 2022
bc4097c
minor miss
zyy98 Jul 22, 2022
ca0d775
Merge branch 'master' into error_msg
zyy98 Jul 29, 2022
80aa94f
no gvk in subobject
zyy98 Aug 8, 2022
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
3 changes: 2 additions & 1 deletion go/fn/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,12 @@ func (e *errKubeObjectFields) Error() string {

// errSubObjectFields raises if the SubObject operation panics.
type errSubObjectFields struct {
obj *SubObject
fields []string
}

func (e *errSubObjectFields) Error() string {
return fmt.Sprintf("SubObject has unmatched field type: `%v", strings.Join(e.fields, "/"))
return fmt.Sprintf("SubObject has unmatched field type: `%v`, relative path to parent kubeObject(group=%v, version=%v, kind=%v) is `%v`", strings.Join(e.fields, "/"), e.obj.gvk.Group, e.obj.gvk.Version, e.obj.gvk.Kind, strings.Join(e.obj.nodePath, "/"))
}

type errResultEnd struct {
Expand Down
84 changes: 60 additions & 24 deletions go/fn/object.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ func ParseKubeObject(in []byte) (*KubeObject, error) {
func (o *SubObject) GetOrDie(ptr interface{}, fields ...string) {
_, err := o.Get(ptr, fields...)
if err != nil {
panic(errSubObjectFields{fields: fields})
panic(errSubObjectFields{obj: o, fields: fields})
}
}

Expand All @@ -69,7 +69,7 @@ func (o *SubObject) NestedBool(fields ...string) (bool, bool, error) {
func (o *SubObject) NestedBoolOrDie(fields ...string) bool {
val, _, err := o.NestedBool(fields...)
if err != nil {
panic(errSubObjectFields{fields: fields})
panic(errSubObjectFields{obj: o, fields: fields})
}
return val
}
Expand All @@ -86,7 +86,7 @@ func (o *SubObject) NestedString(fields ...string) (string, bool, error) {
func (o *SubObject) NestedStringOrDie(fields ...string) string {
val, _, err := o.NestedString(fields...)
if err != nil {
panic(errSubObjectFields{fields: fields})
panic(errSubObjectFields{obj: o, fields: fields})
}
return val
}
Expand All @@ -103,7 +103,7 @@ func (o *SubObject) NestedFloat64(fields ...string) (float64, bool, error) {
func (o *SubObject) NestedFloat64OrDie(fields ...string) float64 {
val, _, err := o.NestedFloat64(fields...)
if err != nil {
panic(errSubObjectFields{fields: fields})
panic(errSubObjectFields{obj: o, fields: fields})
}
return val
}
Expand All @@ -120,7 +120,7 @@ func (o *SubObject) NestedInt64(fields ...string) (int64, bool, error) {
func (o *SubObject) NestedInt64OrDie(fields ...string) int64 {
val, _, err := o.NestedInt64(fields...)
if err != nil {
panic(errSubObjectFields{fields: fields})
panic(errSubObjectFields{obj: o, fields: fields})
}
return val
}
Expand All @@ -141,7 +141,7 @@ func (o *SubObject) NestedSlice(fields ...string) (SliceSubObjects, bool, error)
}
sliceVal, found, err := mapVariant.GetNestedSlice(fields[len(fields)-1])
if err != nil {
panic(errSubObjectFields{fields: fields})
panic(errSubObjectFields{obj: o, fields: fields})
}
if !found {
return nil, found, nil
Expand All @@ -152,7 +152,15 @@ func (o *SubObject) NestedSlice(fields ...string) (SliceSubObjects, bool, error)
}
var val []*SubObject
for _, obj := range objects {
val = append(val, &SubObject{obj: obj})
if o.nodePath == nil {
oGroup, oVersion := ParseGroupVersion(o.GetString("apiVersion"))
val = append(val, &SubObject{obj: obj, gvk: &ResourceIdentifier{
Group: oGroup,
Version: oVersion,
Kind: o.GetString("kind"),
}, nodePath: append(o.nodePath, fields...)})
}
val = append(val, &SubObject{obj: obj, gvk: o.gvk, nodePath: append(o.nodePath, fields...)})
}
return val, true, nil
}
Expand All @@ -164,7 +172,7 @@ func (o *SubObject) NestedSlice(fields ...string) (SliceSubObjects, bool, error)
func (o *SubObject) NestedSliceOrDie(fields ...string) SliceSubObjects {
val, _, err := o.NestedSlice(fields...)
if err != nil {
panic(errSubObjectFields{fields: fields})
panic(errSubObjectFields{obj: o, fields: fields})
}
return val
}
Expand All @@ -184,7 +192,7 @@ func (o *SubObject) NestedStringMap(fields ...string) (map[string]string, bool,
func (o *SubObject) NestedStringMapOrDie(fields ...string) map[string]string {
val, _, err := o.NestedStringMap(fields...)
if err != nil {
panic(errSubObjectFields{fields: fields})
panic(errSubObjectFields{obj: o, fields: fields})
}
return val
}
Expand All @@ -204,7 +212,7 @@ func (o *SubObject) NestedStringSlice(fields ...string) ([]string, bool, error)
func (o *SubObject) NestedStringSliceOrDie(fields ...string) []string {
val, _, err := o.NestedStringSlice(fields...)
if err != nil {
panic(errSubObjectFields{fields: fields})
panic(errSubObjectFields{obj: o, fields: fields})
}
return val
}
Expand All @@ -213,7 +221,7 @@ func (o *SubObject) NestedStringSliceOrDie(fields ...string) []string {
// encounters any error.
func (o *SubObject) RemoveNestedFieldOrDie(fields ...string) {
if _, err := o.RemoveNestedField(fields...); err != nil {
panic(errSubObjectFields{fields: fields})
panic(errSubObjectFields{obj: o, fields: fields})
}
}

Expand Down Expand Up @@ -318,7 +326,7 @@ func (o *SubObject) Get(ptr interface{}, fields ...string) (bool, error) {
// It will panic if it encounters any error.
func (o *SubObject) SetOrDie(val interface{}, fields ...string) {
if err := o.SetNestedField(val, fields...); err != nil {
panic(errSubObjectFields{fields: fields})
panic(errSubObjectFields{obj: o, fields: fields})
}
}

Expand Down Expand Up @@ -419,7 +427,7 @@ func (o *SubObject) SetNestedField(val interface{}, fields ...string) error {
func (o *SubObject) SetNestedIntOrDie(value int, fields ...string) {
err := o.SetNestedInt(value, fields...)
if err != nil {
panic(errSubObjectFields{fields: fields})
panic(errSubObjectFields{obj: o, fields: fields})
}
}

Expand All @@ -432,7 +440,7 @@ func (o *SubObject) SetNestedInt(value int, fields ...string) error {
func (o *SubObject) SetNestedBoolOrDie(value bool, fields ...string) {
err := o.SetNestedBool(value, fields...)
if err != nil {
panic(errSubObjectFields{fields: fields})
panic(errSubObjectFields{obj: o, fields: fields})
}
}

Expand All @@ -445,7 +453,7 @@ func (o *SubObject) SetNestedBool(value bool, fields ...string) error {
func (o *SubObject) SetNestedStringOrDie(value string, fields ...string) {
err := o.SetNestedString(value, fields...)
if err != nil {
panic(errSubObjectFields{fields: fields})
panic(errSubObjectFields{obj: o, fields: fields})
}
}

Expand All @@ -458,7 +466,7 @@ func (o *SubObject) SetNestedString(value string, fields ...string) error {
func (o *SubObject) SetNestedStringMapOrDie(value map[string]string, fields ...string) {
err := o.SetNestedStringMap(value, fields...)
if err != nil {
panic(errSubObjectFields{fields: fields})
panic(errSubObjectFields{obj: o, fields: fields})
}
}

Expand All @@ -471,7 +479,7 @@ func (o *SubObject) SetNestedStringMap(value map[string]string, fields ...string
func (o *SubObject) SetNestedStringSliceOrDie(value []string, fields ...string) {
err := o.SetNestedStringSlice(value, fields...)
if err != nil {
panic(errSubObjectFields{fields: fields})
panic(errSubObjectFields{obj: o, fields: fields})
}
}

Expand Down Expand Up @@ -532,7 +540,7 @@ func (o *KubeObject) SetHeadComment(comment string, fields ...string) error {
// be a pointer to a typed object. It will panic if it encounters an error.
func (o *SubObject) AsOrDie(ptr interface{}) {
if err := o.As(ptr); err != nil {
panic(errSubObjectFields{fields: nil})
panic(errSubObjectFields{obj: o, fields: nil})
}
}

Expand Down Expand Up @@ -889,11 +897,22 @@ func (o *KubeObject) IsEmpty() bool {
}

func NewEmptyKubeObject() *KubeObject {
return &KubeObject{SubObject{internal.NewMap(nil)}}
return &KubeObject{SubObject: SubObject{obj: internal.NewMap(nil)}}
}

func asKubeObject(obj *internal.MapVariant) *KubeObject {
return &KubeObject{SubObject{obj}}
kind, _, _ := obj.GetNestedString("kind")
apiVersion, _, _ := obj.GetNestedString("apiVersion")
group, version := ParseGroupVersion(apiVersion)
if kind != "" || group != "" || version != "" {
return &KubeObject{SubObject: SubObject{obj: obj, gvk: &ResourceIdentifier{
Group: group,
Version: version,
Kind: kind,
}}}
}
// will not go to this line as KubeObject contains GVK for sure
return &KubeObject{SubObject: SubObject{obj: obj}}
}

func (o *KubeObject) node() *internal.MapVariant {
Expand All @@ -902,17 +921,20 @@ func (o *KubeObject) node() *internal.MapVariant {

func rnodeToKubeObject(rn *yaml.RNode) *KubeObject {
mapVariant := internal.NewMap(rn.YNode())
return &KubeObject{SubObject{mapVariant}}
return asKubeObject(mapVariant)
//return &KubeObject{SubObject: SubObject{obj: mapVariant}}
}

// SubObject represents a map within a KubeObject
type SubObject struct {
obj *internal.MapVariant
obj *internal.MapVariant
gvk *ResourceIdentifier // the identifier
nodePath []string // the relative path to the kubeObject
}

func (o *SubObject) UpsertMap(k string) *SubObject {
m := o.obj.UpsertMap(k)
return &SubObject{obj: m}
return &SubObject{gvk: o.gvk, nodePath: o.nodePath, obj: m}
}

// GetMap accepts a single key `k` whose value is expected to be a map. It returns
Expand All @@ -924,7 +946,21 @@ func (o *SubObject) GetMap(k string) *SubObject {
if err != nil || !found {
return nil
}
return &SubObject{obj: internal.NewMap(variant.YNode())}
// this SubObject is the root kubeObject
if o.nodePath == nil {
oGroup, oVersion := ParseGroupVersion(o.GetString("apiVersion"))
Copy link
Contributor

Choose a reason for hiding this comment

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

As we discussed offline. o is not a KubeObject, so we should not call the KRM specific functions on it (apiVersion , kind, namespace)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 😱 )?

Copy link
Contributor

@natasha41575 natasha41575 Aug 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I think in the ideal case, we would have the following:

  1. o.gvk gets populated only when the KubeObject is created. I don't think the SubObject can 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.
  2. The SubObject code can't assume we have o.gvk in 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.

return &SubObject{
obj: internal.NewMap(variant.YNode()),
gvk: &ResourceIdentifier{
Group: oGroup,
Version: oVersion,
Kind: o.GetString("kind"),
},
nodePath: append(o.nodePath, k),
}
}
// otherwise store the SubObject's relative path
return &SubObject{obj: internal.NewMap(variant.YNode()), gvk: o.gvk, nodePath: append(o.nodePath, k)}
}

// GetBool accepts a single key `k` whose value is expected to be a boolean. It returns
Expand Down
83 changes: 83 additions & 0 deletions go/fn/object_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,89 @@ import (
"github.com/stretchr/testify/assert"
)

// processError recover the error catch by panic
func processError(t *testing.T, expected string, err error) {
v := recover()
if v != nil {
switch t := v.(type) {
case errKubeObjectFields:
err = &t
case *errKubeObjectFields:
err = t
case errSubObjectFields:
err = &t
case *errSubObjectFields:
err = t
case errResultEnd:
err = &t
case *errResultEnd:
err = t
case ErrAttemptToTouchUpstreamIdentifier:
err = &t
case *ErrAttemptToTouchUpstreamIdentifier:
err = t
case ErrInternalAnnotation:
err = &t
case *ErrInternalAnnotation:
err = t
default:
panic(v)
}
}
assert.Equal(t, err.Error(), expected)
}

func TestWrongKRM(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

input := `
apiVersion: apps/v1
kind: StatefulSet
metadata:
name: web
spec:
selector:
matchLabels:
app: nginx # has to match .spec.template.metadata.labels
serviceName: "nginx"
replicas: 3 # by default is 1
minReadySeconds: 10 # by default is 0
volumeClaimTemplates:
- metadata:
name: www
labels:
key: www
`
parseInput, err := ParseKubeObject([]byte(input))
expectedKRMError := "SubObject has unmatched field type: `metadata`, relative path to parent kubeObject(group=apps, version=v1, kind=StatefulSet) is ``"
defer processError(t, expectedKRMError, err)
_, _, err = parseInput.NestedSlice("metadata")
}

func TestWrongKRMWithSubObject(t *testing.T) {
input := `
apiVersion: apps/v1
kind: StatefulSet
metadata:
name: web
spec:
selector:
matchLabels:
app: nginx # has to match .spec.template.metadata.labels
serviceName: "nginx"
replicas: 3 # by default is 1
minReadySeconds: 10 # by default is 0
volumeClaimTemplates:
- metadata:
name: www
labels:
key: www
`
parseInput, err := ParseKubeObject([]byte(input))
expectedKRMError := "SubObject has unmatched field type: `serviceName`, relative path to parent kubeObject(group=apps, version=v1, kind=StatefulSet) is `spec`"
defer processError(t, expectedKRMError, err)
subObj := parseInput.GetMap("spec")
_, _, err = subObj.NestedSlice("serviceName")
}

func TestIsGVK(t *testing.T) {
input := []byte(`
apiVersion: apps/v3
Expand Down
2 changes: 1 addition & 1 deletion go/fn/resourcelist.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ func ParseResourceList(in []byte) (*ResourceList, error) {
// toYNode converts the ResourceList to the yaml.Node representation.
func (rl *ResourceList) toYNode() (*yaml.Node, error) {
reMap := internal.NewMap(nil)
reObj := &KubeObject{SubObject{reMap}}
reObj := &KubeObject{SubObject: SubObject{obj: reMap}}
reObj.SetAPIVersion(kio.ResourceListAPIVersion)
reObj.SetKind(kio.ResourceListKind)

Expand Down