Skip to content

Commit

Permalink
chore: comments and refactor
Browse files Browse the repository at this point in the history 
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
  • Loading branch information
@phisco
phisco committed Oct 23, 2023
1 parent 10e4957 commit 8bf644b
Show file tree
Hide file tree
Showing 8 changed files with 154 additions and 95 deletions.
66 changes: 43 additions & 23 deletions cmd/crank/beta/trace/internal/printer/default.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ import (
)

const (
errFmtCannotWriteHeader = "cannot write header: %s"
errFmtCannotWriteRow = "cannot write row: %s"
errFmtCannotFlushTabWriter = "cannot flush tab writer: %s"
errWriteHeader = "cannot write header"
errWriteRow = "cannot write row"
errFlushTabWriter = "cannot flush tab writer"
)

// DefaultPrinter defines the DefaultPrinter configuration
Expand Down Expand Up @@ -71,7 +71,7 @@ func (p *DefaultPrinter) Print(w io.Writer, root *resource.Resource) error {
status: "STATUS",
}
if _, err := fmt.Fprintln(tw, headers.String()); err != nil {
return errors.Errorf(errFmtCannotWriteHeader, err)
return errors.Wrap(err, errWriteHeader)
}

type queueItem struct {
Expand All @@ -81,30 +81,35 @@ func (p *DefaultPrinter) Print(w io.Writer, root *resource.Resource) error {
prefix string
}

// Initialize LIFO queue with root element
// Initialize LIFO queue with root element to traverse the tree depth-first,
// enqueuing children in reverse order so that they are dequeued in the
// right order w.r.t. the way they are defined by the resources.
queue := []*queueItem{{resource: root}}

for len(queue) > 0 {
// Dequeue first element
var item *queueItem
l := len(queue)
item, queue = queue[l-1], queue[:l-1]
item, queue = queue[l-1], queue[:l-1] // Pop the last element

// Choose the right prefix
// Build the name of the current node, prepending the required prefix to
// show the tree structure
name := strings.Builder{}

childPrefix := item.prefix
if item.depth > 0 {
if item.isLast {
name.WriteString(item.prefix + "└─ ")
childPrefix += " "
} else {
name.WriteString(item.prefix + "├─ ")
childPrefix += "│ "
}
childPrefix := item.prefix // Inherited prefix for all the children of the current node
switch {
case item.depth == 0:
// We don't need a prefix for the root, nor a custom
// prefix for its children
case item.isLast:
name.WriteString(item.prefix + "└─ ")
childPrefix += " "
default:
name.WriteString(item.prefix + "├─ ")
childPrefix += "│ "
}

name.WriteString(fmt.Sprintf("%s/%s", item.resource.Unstructured.GetKind(), item.resource.Unstructured.GetName()))

// Append the namespace if it's not empty
if item.resource.Unstructured.GetNamespace() != "" {
name.WriteString(fmt.Sprintf(" (%s)", item.resource.Unstructured.GetNamespace()))
}
Expand All @@ -117,46 +122,61 @@ func (p *DefaultPrinter) Print(w io.Writer, root *resource.Resource) error {
status: status,
}
if _, err := fmt.Fprintln(tw, row.String()); err != nil {
return errors.Errorf(errFmtCannotWriteRow, err)
return errors.Wrap(err, errWriteRow)
}

// Enqueue the children of the current node
// Enqueue the children of the current node in reverse order to ensure
// that they are dequeued from the LIFO queue in the same order w.r.t.
// the way they are defined by the resources.
for idx := len(item.resource.Children) - 1; idx >= 0; idx-- {
isLast := idx == len(item.resource.Children)-1
queue = append(queue, &queueItem{resource: item.resource.Children[idx], depth: item.depth + 1, isLast: isLast, prefix: childPrefix})
}
}

if err := tw.Flush(); err != nil {
return errors.Errorf(errFmtCannotFlushTabWriter, err)
return errors.Wrap(err, errFlushTabWriter)
}

return nil
}

// getResourceStatus returns the status of the resource.
func getResourceStatus(r *resource.Resource, wide bool) (ready string, synced string, status string) { //nolint:gocyclo // it's just a switch
func getResourceStatus(r *resource.Resource, wide bool) (ready string, synced string, status string) {
readyCond := r.GetCondition(xpv1.TypeReady)
syncedCond := r.GetCondition(xpv1.TypeSynced)
var m string
switch {
case r.Error != nil:
// if there is an error we want to show it
status = "Error"
m = r.Error.Error()
case readyCond.Status == corev1.ConditionTrue && syncedCond.Status == corev1.ConditionTrue:
// if both are true we want to show the ready reason only
status = string(readyCond.Reason)

// The following cases are for when one of the conditions is false,
// prioritizing readiness over synced in case of issues.
case readyCond.Status == corev1.ConditionFalse:
status = string(readyCond.Reason)
m = readyCond.Message
case syncedCond.Status == corev1.ConditionFalse:
status = string(syncedCond.Reason)
m = syncedCond.Message
case syncedCond.Status != corev1.ConditionTrue && readyCond.Status != corev1.ConditionTrue:

default:
// both are unknown or unset, let's try showing the ready reason, probably empty
status = string(readyCond.Reason)
m = readyCond.Message
}

// Crop the message to the last 64 characters if it's too long and we are
// not in wide mode
if !wide && len(m) > 64 {
m = "..." + m[len(m)-64:]
}

// Append the message to the status if it's not empty
if m != "" {
status = fmt.Sprintf("%s: %s", status, m)
}
Expand Down
17 changes: 9 additions & 8 deletions cmd/crank/beta/trace/internal/printer/default_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,15 @@ func TestDefaultPrinter(t *testing.T) {
want: want{
// Note: Use spaces instead of tabs for intendation
output: `
NAME READY SYNCED STATUS
ObjectStorage/test-resource (default) True True
└─ XObjectStorage/test-resource-hash (default) True True
├─ Bucket/test-resource-bucket-hash (default) True True
│ ├─ User/test-resource-child-1-bucket-hash (default) False True SomethingWrongHappened: Error with bucket child 1
│ └─ User/test-resource-child-2-bucket-hash (default) False True SomethingWrongHappened: Error with bucket child 2
│ └─ User/test-resource-child-2-1-bucket-hash (default) False True SomethingWrongHappened: Error with bucket child 2-1
└─ User/test-resource-user-hash (default) True True
NAME READY SYNCED STATUS
ObjectStorage/test-resource (default) True True
└─ XObjectStorage/test-resource-hash True True
├─ Bucket/test-resource-bucket-hash True True
│ ├─ User/test-resource-child-1-bucket-hash False True SomethingWrongHappened: Error with bucket child 1
│ ├─ User/test-resource-child-mid-bucket-hash True True AllGood
│ └─ User/test-resource-child-2-bucket-hash False True SomethingWrongHappened: Error with bucket child 2
│ └─ User/test-resource-child-2-1-bucket-hash False True SomethingWrongHappened: Error with bucket child 2-1
└─ User/test-resource-user-hash True True
`,
err: nil,
},
Expand Down
16 changes: 9 additions & 7 deletions cmd/crank/beta/trace/internal/printer/dot_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,20 @@ func TestPrintDotGraph(t *testing.T) {
dotString: `graph {
n1[label="Name: ObjectStorage/test-resource\nApiVersion: test.cloud/v1alpha1\nNamespace: default\nReady: True\nSynced: True\n",penwidth="2"];
n2[label="Name: XObjectStorage/test-resource-hash\nApiVersion: test.cloud/v1alpha1\nNamespace: default\nReady: True\nSynced: True\n",penwidth="2"];
n3[label="Name: Bucket/test-resource-bucket-hash\nApiVersion: test.cloud/v1alpha1\nNamespace: default\nReady: True\nSynced: True\n",penwidth="2"];
n4[label="Name: User/test-resource-user-hash\nApiVersion: test.cloud/v1alpha1\nNamespace: default\nReady: True\nSynced: True\n",penwidth="2"];
n5[label="Name: User/test-resource-child-1-bucket-hash\nApiVersion: test.cloud/v1alpha1\nNamespace: default\nReady: False\nSynced: True\n",penwidth="2"];
n6[label="Name: User/test-resource-child-2-bucket-hash\nApiVersion: test.cloud/v1alpha1\nNamespace: default\nReady: False\nSynced: True\n",penwidth="2"];
n7[label="Name: User/test-resource-child-2-1-bucket-hash\nApiVersion: test.cloud/v1alpha1\nNamespace: default\nReady: False\nSynced: True\n",penwidth="2"];
n2[label="Name: XObjectStorage/test-resource-hash\nApiVersion: test.cloud/v1alpha1\nReady: True\nSynced: True\n",penwidth="2"];
n3[label="Name: Bucket/test-resource-bucket-hash\nApiVersion: test.cloud/v1alpha1\nReady: True\nSynced: True\n",penwidth="2"];
n4[label="Name: User/test-resource-user-hash\nApiVersion: test.cloud/v1alpha1\nReady: True\nSynced: True\n",penwidth="2"];
n5[label="Name: User/test-resource-child-1-bucket-hash\nApiVersion: test.cloud/v1alpha1\nReady: False\nSynced: True\n",penwidth="2"];
n6[label="Name: User/test-resource-child-mid-bucket-hash\nApiVersion: test.cloud/v1alpha1\nReady: True\nSynced: True\n",penwidth="2"];
n7[label="Name: User/test-resource-child-2-bucket-hash\nApiVersion: test.cloud/v1alpha1\nReady: False\nSynced: True\n",penwidth="2"];
n8[label="Name: User/test-resource-child-2-1-bucket-hash\nApiVersion: test.cloud/v1alpha1\nReady: False\nSynced: True\n",penwidth="2"];
n1--n2;
n2--n3;
n2--n4;
n3--n5;
n3--n6;
n6--n7;
n3--n7;
n7--n8;
}
`,
Expand Down
36 changes: 30 additions & 6 deletions cmd/crank/beta/trace/internal/printer/json_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ func TestJSONPrinter(t *testing.T) {
"kind": "XObjectStorage",
"metadata": {
"name": "test-resource-hash",
"namespace": "default"
"namespace": ""
},
"status": {
"conditions": [
Expand All @@ -106,7 +106,7 @@ func TestJSONPrinter(t *testing.T) {
"kind": "Bucket",
"metadata": {
"name": "test-resource-bucket-hash",
"namespace": "default"
"namespace": ""
},
"status": {
"conditions": [
Expand All @@ -130,7 +130,7 @@ func TestJSONPrinter(t *testing.T) {
"kind": "User",
"metadata": {
"name": "test-resource-child-1-bucket-hash",
"namespace": "default"
"namespace": ""
},
"status": {
"conditions": [
Expand All @@ -149,13 +149,37 @@ func TestJSONPrinter(t *testing.T) {
}
}
},
{
"object": {
"apiVersion": "test.cloud/v1alpha1",
"kind": "User",
"metadata": {
"name": "test-resource-child-mid-bucket-hash",
"namespace": ""
},
"status": {
"conditions": [
{
"status": "True",
"type": "Synced"
},
{
"type": "Ready",
"status": "True",
"lastTransitionTime": null,
"reason": "AllGood"
}
]
}
}
},
{
"object": {
"apiVersion": "test.cloud/v1alpha1",
"kind": "User",
"metadata": {
"name": "test-resource-child-2-bucket-hash",
"namespace": "default"
"namespace": ""
},
"status": {
"conditions": [
Expand All @@ -180,7 +204,7 @@ func TestJSONPrinter(t *testing.T) {
"kind": "User",
"metadata": {
"name": "test-resource-child-2-1-bucket-hash",
"namespace": "default"
"namespace": ""
},
"status": {
"conditions": [
Expand Down Expand Up @@ -209,7 +233,7 @@ func TestJSONPrinter(t *testing.T) {
"kind": "User",
"metadata": {
"name": "test-resource-user-hash",
"namespace": "default"
"namespace": ""
},
"status": {
"conditions": [
Expand Down
25 changes: 16 additions & 9 deletions cmd/crank/beta/trace/internal/printer/printer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@ import (
)

// Returns an unstructured that has basic fields set to be used by other tests.
func DummyManifest(kind, name, syncedStatus string, readyCond *xpv1.Condition) unstructured.Unstructured {
func DummyManifest(kind, name, namespace, syncedStatus string, readyCond *xpv1.Condition) unstructured.Unstructured {
m := unstructured.Unstructured{
Object: map[string]interface{}{
"apiVersion": "test.cloud/v1alpha1",
"kind": kind,
"metadata": map[string]interface{}{
"name": name,
"namespace": "default",
"namespace": namespace,
},
"status": map[string]interface{}{
"conditions": []interface{}{
Expand All @@ -51,41 +51,48 @@ func DummyManifest(kind, name, syncedStatus string, readyCond *xpv1.Condition) u

func GetComplexResource() *resource.Resource {
return &resource.Resource{
Unstructured: DummyManifest("ObjectStorage", "test-resource", "True", &xpv1.Condition{
Unstructured: DummyManifest("ObjectStorage", "test-resource", "default", "True", &xpv1.Condition{
Type: "Ready",
Status: "True",
}),
Children: []*resource.Resource{
{
Unstructured: DummyManifest("XObjectStorage", "test-resource-hash", "True", &xpv1.Condition{
Unstructured: DummyManifest("XObjectStorage", "test-resource-hash", "", "True", &xpv1.Condition{
Type: "Ready",
Status: "True",
}),
Children: []*resource.Resource{
{
Unstructured: DummyManifest("Bucket", "test-resource-bucket-hash", "True", &xpv1.Condition{
Unstructured: DummyManifest("Bucket", "test-resource-bucket-hash", "", "True", &xpv1.Condition{
Type: "Ready",
Status: "True",
}),
Children: []*resource.Resource{
{
Unstructured: DummyManifest("User", "test-resource-child-1-bucket-hash", "True", &xpv1.Condition{
Unstructured: DummyManifest("User", "test-resource-child-1-bucket-hash", "", "True", &xpv1.Condition{
Type: "Ready",
Status: "False",
Reason: "SomethingWrongHappened",
Message: "Error with bucket child 1",
}),
},
{
Unstructured: DummyManifest("User", "test-resource-child-2-bucket-hash", "True", &xpv1.Condition{
Unstructured: DummyManifest("User", "test-resource-child-mid-bucket-hash", "", "True", &xpv1.Condition{
Type: "Ready",
Status: "True",
Reason: "AllGood",
}),
},
{
Unstructured: DummyManifest("User", "test-resource-child-2-bucket-hash", "", "True", &xpv1.Condition{
Type: "Ready",
Reason: "SomethingWrongHappened",
Status: "False",
Message: "Error with bucket child 2",
}),
Children: []*resource.Resource{
{
Unstructured: DummyManifest("User", "test-resource-child-2-1-bucket-hash", "True", &xpv1.Condition{
Unstructured: DummyManifest("User", "test-resource-child-2-1-bucket-hash", "", "True", &xpv1.Condition{
Type: "Ready",
Reason: "SomethingWrongHappened",
Status: "False",
Expand All @@ -97,7 +104,7 @@ func GetComplexResource() *resource.Resource {
},
},
{
Unstructured: DummyManifest("User", "test-resource-user-hash", "True", &xpv1.Condition{
Unstructured: DummyManifest("User", "test-resource-user-hash", "", "True", &xpv1.Condition{
Type: "Ready",
Status: "True",
}),
Expand Down

0 comments on commit 8bf644b

Please sign in to comment.