[vtctld] Fix accidentally-broken legacy vtctl output format

[ajm188]

This was a regression introduced in #7128.

Description

This restores the original json format of the old vtctl API, which I broke when trying to reimplement the old API in terms of the new. Example (using the local example): first the new, changed output, and then the old, correct output:

❯ vtctlclient -server "localhost:15999" FindAllShardsInKeyspace commerce
{
  "0": {
    "keyspace": "commerce",
    "name": "0",
    "shard": {
      "master_alias": {
        "cell": "zone1",
        "uid": 100
      },
      "master_term_start_time": {
        "seconds": 1610412569,
        "nanoseconds": 995650000
      },
      "is_master_serving": true
    }
  }
}
❯ cd -
~/work/vitess
❯ make build
Mon Jan 11 19:50:24 EST 2021: Building source tree
❯ cd -
~/work/vitess/examples/local
❯ ./scripts/vtctld-down.sh
Stopping vtctld...
❯ ./scripts/vtctld-up.sh
Starting vtctld...
❯ vtctlclient -server "localhost:15999" FindAllShardsInKeyspace commerce
{
  "0": {
    "master_alias": {
      "cell": "zone1",
      "uid": 100
    },
    "master_term_start_time": {
      "seconds": 1610412569,
      "nanoseconds": 995650000
    },
    "is_master_serving": true
  }
}

Related Issue(s)

Checklist

• Should this PR be backported? No (it hasn't been actually released yet)
• Tests were added or are not required Probably should?
• Documentation was added or is not required N/A

Deployment Notes

Impacted Areas in Vitess

Components that this PR will affect:

• Query Serving
• VReplication
• Cluster Management
• Build
• VTAdmin

[doeg]

+1 with the acknowledgement of :

Tests were added or are not required Probably should?

Let me know if I can help with that! 🌻

[setassociative]

Worth calling out that this was confirmed by observing that the fixed output still works with our tooling that expects the old format

[rafael]

LGTM as well. Merging this as is.

One follow up question: are there other places were this code change could have introduced a similar regression? I didn't look in the detail the previous change where the regression was originally introduced.

[ajm188]

Here's the entire diff to vtctl.go that I've made over the 3 PRs. Everything else has been additive to the new interface instead:

diff --git a/go/vt/vtctl/vtctl.go b/go/vt/vtctl/vtctl.go
index 97b7ac8513..e33d20e082 100644
--- a/go/vt/vtctl/vtctl.go
+++ b/go/vt/vtctl/vtctl.go
@@ -1761,20 +1761,29 @@ func commandGetKeyspace(ctx context.Context, wr *wrangler.Wrangler, subFlags *fl
 	}
 
 	keyspace := subFlags.Arg(0)
-	keyspaceInfo, err := wr.TopoServer().GetKeyspace(ctx, keyspace)
+
+	keyspaceInfo, err := wr.VtctldServer().GetKeyspace(ctx, &vtctldatapb.GetKeyspaceRequest{
+		Keyspace: keyspace,
+	})
 	if err != nil {
 		return err
 	}
 	// Pass the embedded proto directly or jsonpb will panic.
-	return printJSON(wr.Logger(), keyspaceInfo.Keyspace)
+	return printJSON(wr.Logger(), keyspaceInfo.Keyspace.Keyspace)
 }
 
 func commandGetKeyspaces(ctx context.Context, wr *wrangler.Wrangler, subFlags *flag.FlagSet, args []string) error {
-	keyspaces, err := wr.TopoServer().GetKeyspaces(ctx)
+	resp, err := wr.VtctldServer().GetKeyspaces(ctx, &vtctldatapb.GetKeyspacesRequest{})
 	if err != nil {
 		return err
 	}
-	wr.Logger().Printf("%v\n", strings.Join(keyspaces, "\n"))
+
+	names := make([]string, len(resp.Keyspaces))
+	for i, ks := range resp.Keyspaces {
+		names[i] = ks.Name
+	}
+
+	wr.Logger().Printf("%v\n", strings.Join(names, "\n"))
 	return nil
 }
 
@@ -2208,11 +2217,14 @@ func commandFindAllShardsInKeyspace(ctx context.Context, wr *wrangler.Wrangler,
 	}
 
 	keyspace := subFlags.Arg(0)
-	result, err := wr.TopoServer().FindAllShardsInKeyspace(ctx, keyspace)
+	result, err := wr.VtctldServer().FindAllShardsInKeyspace(ctx, &vtctldatapb.FindAllShardsInKeyspaceRequest{
+		Keyspace: keyspace,
+	})
 	if err != nil {
 		return err
 	}
-	return printJSON(wr.Logger(), result)
+
+	return printJSON(wr.Logger(), result.Shards)
 }
 
 func commandValidate(ctx context.Context, wr *wrangler.Wrangler, subFlags *flag.FlagSet, args []string) error {

[deepthi]

@ajm188 can you please create a PR to fix this for the 9.0 release branch?

[ajm188]

Yep, I'll have one out tomorrow!