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.

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

Jump to bottom

Add fully qualified resource reference type #351

Closed
wants to merge 3 commits into from
Closed

Add fully qualified resource reference type #351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
29e3f73
Add fully qualified resource reference type
killianmuldoon Sep 15, 2022
6a9e45a
Remove KResourceRef function
killianmuldoon Sep 15, 2022
b730c71
added tests and cleanups
killianmuldoon Sep 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 21 additions & 0 deletions internal/serialize/keyvalues_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,27 @@ No whitespace.`,
})},
want: " pods=[kube-system/kube-dns mi-conf]",
},
{
keysValues: []interface{}{"deployment", klog.ResourceRef{Group: "apps", Kind: "deployment", Version: "v1", Name: "test-name", Namespace: "test-ns"}, "status", "ready"},
want: " deployment=\"apps/v1/deployment/test-ns/test-name\" status=\"ready\"",
},
{
keysValues: []interface{}{"pod", klog.ResourceRef{Kind: "pod", Version: "v1", Name: "test-name", Namespace: "test-ns"}, "status", "ready"},
want: " pod=\"v1/pod/test-ns/test-name\" status=\"ready\"",
},
{
keysValues: []interface{}{"object", klog.ResourceRef{Group: "test-group", Kind: "test-kind", Version: "v1"}, "status", "ready"},
want: " object=\"test-group/v1/test-kind\" status=\"ready\"",
},
{
keysValues: []interface{}{"object", klog.ResourceRef{Name: "test-name", Namespace: "test-ns"}, "status", "ready"},
want: " object=\"test-ns/test-name\" status=\"ready\"",
},
{
keysValues: []interface{}{"object", klog.ResourceRef{Kind: "test-kind", Name: "test-name", Namespace: "test-ns"}, "status", "ready"},
want: " object=\"test-kind/test-ns/test-name\" status=\"ready\"",
},

{
keysValues: []interface{}{"point-1", point{100, 200}, "point-2", emptyPoint},
want: " point-1=\"x=100, y=200\" point-2=\"<panic: value method k8s.io/klog/v2/internal/serialize_test.point.String called using nil *point pointer>\"",
Expand Down
27 changes: 27 additions & 0 deletions k8s_references.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package klog
import (
"fmt"
"reflect"
"strings"

"github.com/go-logr/logr"
)
Expand Down Expand Up @@ -156,3 +157,29 @@ func (ks kobjSlice) process() ([]interface{}, error) {
}
return objectRefs, nil
}

// ResourceRef references a Kubernetes object using Group, Version, Kind, Namespace and Name
type ResourceRef struct {
Copy link

Choose a reason for hiding this comment

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

Can you please check if this can be named as KResourceRef now that there is no func KResourceRef(....) ?

Copy link
Author

Choose a reason for hiding this comment

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

It feels slightly strange that KObj and KRef are functions while this would be a struct, but I'm happy to go either way on this if there's consensus.

Copy link

Choose a reason for hiding this comment

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

Good point. @pohly which one do you think would be more natural to the framework ?

Copy link

@pohly pohly Sep 27, 2022 •
edited

Choose a reason for hiding this comment

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

For the sake of clarity and usability, I prefer just the struct (see comment above) But for the sake of consistency we would have to use a function.

@serathius: what do you think?

Copy link

Choose a reason for hiding this comment

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

I agree with @pohly that struct should be preferred.

Group string `json:"group,omitempty"`
Version string `json:"version,omitempty"`
Kind string `json:"kind,omitempty"`
Namespace string `json:"namespace,omitempty"`
Name string `json:"name,omitempty"`
}

func (r ResourceRef) String() string {
var parts []string
for _, s := range []string{r.Group, r.Version, r.Kind, r.Namespace, r.Name} {
if strings.TrimSpace(s) != "" {
parts = append(parts, s)
}
}
return strings.Join(parts, "/")
}

// MarshalLog ensures that loggers with support for structured output will log
// as a struct by removing the String method via a custom type.
func (r ResourceRef) MarshalLog() interface{} {
type resourceRefWithoutStringFunc ResourceRef
return resourceRefWithoutStringFunc(r)
}
57 changes: 57 additions & 0 deletions klog_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -953,6 +953,63 @@ func TestKRef(t *testing.T) {
}
}

func TestResourceRefString(t *testing.T) {
killianmuldoon marked this conversation as resolved.
Show resolved Hide resolved
tests := []struct {
testname string
group string
version string
kind string
name string
namespace string
want string
}{
{
testname: "with group, version, kind, namespace and name",
group: "test-group",
version: "v0.1.0",
kind: "test-kind",
namespace: "test-ns",
name: "test-name",
want: "test-group/v0.1.0/test-kind/test-ns/test-name",
},
{
testname: "with group, version, namespace and name",
group: "test-group",
version: "v0.1.0",
namespace: "test-ns",
name: "test-name",
want: "test-group/v0.1.0/test-ns/test-name",
},
{
testname: "with group, namespace and name",
group: "test-group",
namespace: "test-ns",
name: "test-name",
want: "test-group/test-ns/test-name",
},
{
testname: "with namespace and name",
namespace: "test-ns",
name: "test-name",
want: "test-ns/test-name",
},
{
testname: "with only name",
name: "test-name",
want: "test-name",
},
}

for _, tt := range tests {
t.Run(tt.testname, func(t *testing.T) {
ref := ResourceRef{tt.group, tt.version, tt.kind, tt.namespace, tt.name}
if ref.String() != tt.want {
t.Errorf("expected %v, got %v", tt.want, ref.String())
}
})
}
}

// Test that InfoS and InfoSDepth work as advertised.
func TestInfoS(t *testing.T) {
defer CaptureState().Restore()
Expand Down