KGLOBAL-1755: Add link error fields to cluster linking list CLI operation by andrewgrantcflt · Pull Request #1435 · confluentinc/cli

Andrew Grant (andrewgrantcflt) · 2022-09-14T14:51:00Z

JIRA

https://confluentinc.atlassian.net/browse/KGLOBAL-1755

Details

Updates the CLI to handle the new link error fields introduced. I tested running the CLI against a cluster in prod that doesnt have the REST changes to make sure it works against clusters without the new changes and thus is backwards compatible.

The cloud SDK was updated in confluentinc/ccloud-sdk-go-v2#53.

Testing

Ran make test after updating the golden files

Checklist

[CRUCIAL] Is the change for CP or CCloud functionalities that are already live in prod?
- yes: ok
- no: DO NOT MERGE until the required functionalites are live in prod

What

References

Test & Review

…tion

go.sum

Brian Strauch (brianstrauch) · 2022-09-14T16:34:16Z

internal/cmd/kafka/command_link_list.go

+	LinkState            string
+	LinkError            string
+	LinkErrorMessage     string


Let's add these fields to confluent kafka link describe too?

The describe command actually maps to getting the link config i.e. adminClient.describeConfigs so we can't return this data for it.

Oh, interesting. Maybe that should've been put in confluent kafka link configuration list.

Yeah I'd probably agree. In fact confluent kafka link list ultimately calls adminClient.describeClusterLinks which is a bit confusing. We might want to do some refactoring at some point.

This would be a breaking change, so I'll add a note about it to our confluent v3 wiki page.

What's the breaking change here?

Renaming confluent kafka link describe to confluent kafka link configuration list

Yeah it would be. We aren't actively working on doing that as of now, just to clarify.

Brian Strauch (brianstrauch) · 2022-09-14T16:36:51Z

test/fixtures/output/kafka/link/list-link-plain.golden

------------+-------------------+-------------------------
-  link-1    | cluster-1         | cluster-2               
-  link-2    | cluster-1         | cluster-2               
+  Link Name | Source Cluster Id | Destination Cluster Id | Link State  |      Link Error      |       Link Error Message        


It seems redundant to be prefixing all of these new fields with "Link"... the user knows they're getting information on links since they ran confluent kafka link list to get this output. Why not call these "State", "Error", and "Error Message"?

We're also running out of horizontal space 😄

Agreed. Let me try and trim those names that start with Link

Andrew Grant (andrewgrantcflt) · 2022-09-14T17:54:00Z

The Mac build is failing with:

make depsFailed in 
00:02
go install github.com/goreleaser/goreleaser@v1.4.1 && \00:01
	go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.44.0 && \00:01
	go install github.com/mitchellh/golicense@v0.2.0 && \00:01
	go install gotest.tools/gotestsum@v1.8.200:02
# golang.org/x/sys/unix00:02
../go/1.18.0/pkg/mod/golang.org/x/sys@v0.0.0-20190312061237-fead79001313/unix/zsyscall_darwin_amd64.go:28:3: //go:linkname must refer to declared function or variable00:02
../go/1.18.0/pkg/mod/golang.org/x/sys@v0.0.0-20190312061237-fead79001313/unix/zsyscall_darwin_amd64.go:43:3: //go:linkname must refer to declared function or variable00:02
../go/1.18.0/pkg/mod/golang.org/x/sys@v0.0.0-20190312061237-fead79001313/unix/zsyscall_darwin_amd64.go:59:3: //go:linkname must refer to declared function or variable00:02
../go/1.18.0/pkg/mod/golang.org/x/sys@v0.0.0-20190312061237-fead79001313/unix/zsyscall_darwin_amd64.go:75:3: //go:linkname must refer to declared function or variable00:02
../go/1.18.0/pkg/mod/golang.org/x/sys@v0.0.0-20190312061237-fead79001313/unix/zsyscall_darwin_amd64.go:90:3: //go:linkname must refer to declared function or variable00:02
../go/1.18.0/pkg/mod/golang.org/x/sys@v0.0.0-20190312061237-fead79001313/unix/zsyscall_darwin_amd64.go:105:3: //go:linkname must refer to declared function or variable00:02
../go/1.18.0/pkg/mod/golang.org/x/sys@v0.0.0-20190312061237-fead79001313/unix/zsyscall_darwin_amd64.go:121:3: //go:linkname must refer to declared function or variable00:02
../go/1.18.0/pkg/mod/golang.org/x/sys@v0.0.0-20190312061237-fead79001313/unix/zsyscall_darwin_amd64.go:136:3: //go:linkname must refer to declared function or variable00:02
../go/1.18.0/pkg/mod/golang.org/x/sys@v0.0.0-20190312061237-fead79001313/unix/zsyscall_darwin_amd64.go:151:3: //go:linkname must refer to declared function or variable00:02
../go/1.18.0/pkg/mod/golang.org/x/sys@v0.0.0-20190312061237-fead79001313/unix/zsyscall_darwin_amd64.go:166:3: //go:linkname must refer to declared function or variable

Brian Strauch (@brianstrauch) any chance you've seen this before?

Andrew Grant (andrewgrantcflt) · 2022-09-14T18:20:04Z

Per https://stackoverflow.com/questions/71507321/go-1-18-build-error-on-mac-unix-syscall-darwin-1-13-go253-golinkname-mus I'll try updating golang.org/x/sys

Brian Strauch (brianstrauch) · 2022-09-14T19:10:37Z

internal/cmd/kafka/command_link_list.go

+	"github.com/spf13/cobra"
+	"strings"


Can you group these imports separately?

Updated to:

import ( "strings" kafkarestv3 "github.com/confluentinc/ccloud-sdk-go-v2/kafkarest/v3" "github.com/spf13/cobra" pcmd "github.com/confluentinc/cli/internal/pkg/cmd" "github.com/confluentinc/cli/internal/pkg/errors" "github.com/confluentinc/cli/internal/pkg/output" )

Brian Strauch (brianstrauch) · 2022-09-14T19:11:11Z

internal/cmd/kafka/command_link_list.go

+	LinkState            string
+	LinkError            string
+	LinkErrorMessage     string


Renaming confluent kafka link describe to confluent kafka link configuration list

Brian Strauch (brianstrauch) · 2022-09-14T19:12:55Z

internal/cmd/kafka/command_link_list.go

+	camelToSpacedListFields := camelToSpaced(listFields)
+	var humanLabels []string
+	for _, humanLabel := range camelToSpacedListFields {
+		humanLabels = append(humanLabels, strings.TrimPrefix(humanLabel, "Link "))


We should remove the prefix in the JSON and YAML output too (and in the struct fields, so this edge case doesn't need to be hardcoded)

Are programs supposed to consume the JSON and YAML output? I'm wondering if we should actually leave Link in tbh otherwise we become inconsistent with basically all the other APIs (REST, AdminClient, RPC).

For just the human readable part I understand wanting to remove the redundancy but if programs are supposed to parse the JSON and YAML I think I'd prefer to stay consistent with the other interfaces.

+1 if this is how the fields are named in the API

KGLOBAL-1755: Add link error fields to cluster linking list CLI opera…

c09be79

…tion

Andrew Grant (andrewgrantcflt) requested a review from a team as a code owner September 14, 2022 14:51

Brian Strauch (brianstrauch) reviewed Sep 14, 2022

View reviewed changes

Andrew Grant (andrewgrantcflt) added 2 commits September 14, 2022 12:44

Run go mod tidy

bf4e265

Address PR feedback

cb1b513

Update golang.org/x/sys to fix build failure

56e2c08

Brian Strauch (brianstrauch) reviewed Sep 14, 2022

View reviewed changes

Brian Strauch (brianstrauch) approved these changes Sep 14, 2022

View reviewed changes

Change import ordering

7cd88bd

Andrew Grant (andrewgrantcflt) merged commit 04a1abc into main Sep 14, 2022

Andrew Grant (andrewgrantcflt) deleted the KGLOBAL-1755 branch September 14, 2022 20:43

Conversation

Andrew Grant (andrewgrantcflt) commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

JIRA

Details

Testing

Checklist

What

References

Test & Review

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Brian Strauch (brianstrauch) Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andrew Grant (andrewgrantcflt) Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andrew Grant (andrewgrantcflt) commented Sep 14, 2022

Uh oh!

Andrew Grant (andrewgrantcflt) commented Sep 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Andrew Grant (andrewgrantcflt) commented Sep 14, 2022 •

edited

Loading

Brian Strauch (brianstrauch) Sep 14, 2022 •

edited

Loading

Andrew Grant (andrewgrantcflt) Sep 14, 2022 •

edited

Loading