Add cluster commands `get` #4

zymap · 2019-08-26T10:02:21Z

Master Issue: #2

pkg/ctl/cluster/get_test.go

sijie

Let's use ClusterData rather than ClusterConfiguration. because configuration is a bit confused especially there are endpoints to fetch configuration.

sijie · 2019-08-26T18:34:24Z

pkg/ctl/cluster/create.go

 			"p",
 			[]string{""},
 			"Cluster to be registered as a peer-cluster of this cluster.")
+		cobra.MarkFlagRequired(flagSet, "broker-url")


I don't think we should set broker-url to required directly. The constraint should be "at least one of the options should be set"

➜ pulsar git:(pulsar_reader) ✗ bin/pulsar-admin clusters create --broker-url localhost:8080 testhello The following option is required: --url Provisions a new cluster. This operation requires Pulsar super-user privileges Usage: create [options] cluster-name Options: --broker-url broker-service-url --broker-url-secure broker-service-url for secure connection * --url service-url --url-secure service-url for secure connection

So the Java admin client is wrong too? Is this need to change in java admin client?

yes java admin client needs to be improved as well. but let's first do this in pulsarctl first. we can contribute the changes back to pulsar-admin when we complete pulsarctl

pkg/ctl/cluster/get.go

sijie · 2019-08-26T18:42:46Z

pkg/ctl/cluster/get.go

+		return doGetClusterConfiguration(vc, clusterName)
+	})
+
+	vc.FlagSetGroup.InFlagSet("ClusterName", func(flagSet *pflag.FlagSet) {


don't introduce a --cluster-name option. use NameArg for cluster-name. keep the behavior as same as other commands in the same command group.

get <cluster_name>

create [options] <cluster_name>

sijie · 2019-08-26T18:52:45Z

pkg/ctl/cluster/get.go

+	vc.SetDescription(
+		"get",
+		"Get the configuration data for the specified cluster",
+		"This command is used for getting the configuration data of the specified cluster.",


I think we should try to improve how we write the long description.

It should contains:

a few sentences describe what this command is used for.

a section about the output and example of the output.

a section of examples how to use this command (if applicable).

a section of describing what kind of permissions are required for executing this command.

Please work with Yu or Jennifer to come up a standard for this.

@jennifer88huang since you are on call this week, could u pls help review?

USED FOR: This command is used for getting the cluster data of the specified cluster. PERMISSION: This command only admin can use. EXAMPLE: { serviceUrl : http://localhost:8080, serviceUrlTls : https://localhost:8080, brokerServiceUrl: pulsar://localhost:6650, brokerServiceUrlTls: pulsar+ssl://localhost:6650, peerClusterNames: "" } Usage: pulsarctl clusters get [flags] Aliases: get, get Common flags: -s, --admin-service-url string The admin web service url that pulsarctl connects to. (default "http://localhost:8080") -C, --color string toggle colorized logs (true,false,fabulous) (default "true") -h, --help help for this command -v, --verbose int set log level, use 0 to silence, 4 for debugging (default 3) Use 'pulsarctl clusters get [command] --help' for more information about a command.

Currently, the long description will show like this. Any suggestion? @sijie @jennifer88huang

@zymap :

examples should be used for describing the possible combinations of how to run this command. for example, this is the examples of kubectl get.

Examples: # List all pods in ps output format. kubectl get pods # List all pods in ps output format with more information (such as node name). kubectl get pods -o wide # List a single replication controller with specified NAME in ps output format. kubectl get replicationcontroller web # List deployments in JSON output format, in the "v1" version of the "apps" API group: kubectl get deployments.v1.apps -o json # List a single pod in JSON output format. kubectl get -o json pod web-pod-13je7 # List a pod identified by type and name specified in "pod.yaml" in JSON output format. kubectl get -f pod.yaml -o json # List resources from a directory with kustomization.yaml - e.g. dir/kustomization.yaml. kubectl get -k dir/ # Return only the phase value of the specified pod. kubectl get -o template pod/web-pod-13je7 --template={{.status.phase}} # List resource information in custom columns. kubectl get pod test-pod -o custom-columns=CONTAINER:.spec.containers[0].name,IMAGE:.spec.containers[0].image # List all replication controllers and services together in ps output format. kubectl get rc,services # List one or more resources by their type and names. kubectl get rc/web service/frontend pods/web-pod-13je7

You need a section to document the output. You should document the output when the command runs successfully and when it fails to run.

USED FOR: <used-for-description> REQUIRED PERMISSIONS: <required-permissions> EXAMPLES: // if the command is simple, the examples can be ommitted <list-the-examples-of-commands> OUTPUT: <describe the output of the command>

@sijie Got it, thanks for the example.

sijie · 2019-08-26T19:00:12Z

pkg/ctl/cluster/test.go

+	"github.com/streamnative/pulsarctl/pkg/cmdutils"
+)
+
+func TestCommands(newVerb func(cmd *cmdutils.VerbCmd), args []string) (out *bytes.Buffer, err error)  {


Is this only used for testing cluster commands? If so, let's use a more specific name like TestClusterCommands rather than a general name.

sijie · 2019-08-28T00:15:46Z

pkg/ctl/cluster/get.go

+	return err
+}
+
+func concat(join string) string {


Why not define a Struct for the long description and have a common method to get the string for long description? So you don't need to write the concat logic every time you introduce a new command.

sijie · 2019-08-28T00:16:08Z

pkg/ctl/cluster/get.go

+}
+
+func concat(join string) string {
+	return "USED FOR:" + join + "\t" + commandUsedFor  + join +


Use four spaces instead of \t

sijie · 2019-08-28T00:16:49Z

pkg/ctl/cluster/get.go

+		"    brokerServiceUrlTls: pulsar+ssl://localhost:6650, \n" +
+		"    peerClusterNames: \"\" \n" +
+		"}\n"
+var commandPermission = "This command only admin can use."


Suggested change

var commandPermission = "This command only admin can use."

var commandPermission = "This command requires super-user permissions."

sijie · 2019-08-28T00:18:14Z

pkg/ctl/cluster/get.go

+
+
+var commandUsedFor = "This command is used for getting the cluster data of the specified cluster."
+var commandExample =


This is not example. It is an example of output. Also you didn't document the error case as I suggested in my previous comment.

zymap · 2019-08-28T07:39:18Z

@sijie I update the long description of the command get. Any suggestion? @jennifer88huang

USED FOR:
    This command is used for getting the cluster data of the specified cluster.

REQUIRED PERMISSION:
    This command requires super-user permissions.

EXAMPLES:
    #getting the <cluster-name> data
    pulsarctl clusters get <cluster-name>

OUTPUT:
    #normal output
    {
      "serviceUrl": "http://localhost:8080",
      "serviceUrlTls": "",
      "brokerServiceUrl": "pulsar://localhost:6650",
      "brokerServiceUrlTls": "",
      "peerClusterNames": null
    }

    #output of doesn't specified a cluster name
    only one argument is allowed to be used as a name

Usage: pulsarctl clusters get [flags]

Aliases: get, get

Common flags:
  -s, --admin-service-url string   The admin web service url that pulsarctl connects to. (default "http://localhost:8080")
  -C, --color string               toggle colorized logs (true,false,fabulous) (default "true")
  -h, --help                       help for this command
  -v, --verbose int                set log level, use 0 to silence, 4 for debugging (default 3)

Use 'pulsarctl clusters get [command] --help' for more information about a command.

Add cluster commands `get`

Add cluster commands get

b2f8089

zymap requested a review from wolfstudy August 26, 2019 10:02

zymap self-assigned this Aug 26, 2019

wolfstudy suggested changes Aug 26, 2019

View reviewed changes

pkg/ctl/cluster/get_test.go Outdated Show resolved Hide resolved

zymap added 2 commits August 26, 2019 18:15

Add ci

c5a5be7

Remove unused dependencies

7858d11

zymap changed the title ~~[WIP] Add cluster commands get~~ Add cluster commands get Aug 26, 2019

zymap and others added 2 commits August 26, 2019 20:25

Remove .github

a214a84

Merge remote-tracking branch 'sn/master' into cluster_get

8206e31

sijie requested changes Aug 26, 2019

View reviewed changes

sijie added area/pulsar/clusters type/task Indicates a chore or a small item of work labels Aug 26, 2019

sijie mentioned this pull request Aug 26, 2019

pulsarctl MVP #2

Closed

29 tasks

sijie added this to the 0.0.1 milestone Aug 26, 2019

zymap added 2 commits August 27, 2019 15:14

Address most comments

9b95d26

Add command description

e619bba

sijie requested changes Aug 28, 2019

View reviewed changes

zymap added 4 commits August 28, 2019 13:20

Add a simple long description of the command get

c183bb9

Update the description of the command get

9cebaf4

Update the long description

1d2d660

Update description

74c7a6c

sijie approved these changes Aug 28, 2019

View reviewed changes

zymap merged commit d5f4639 into master Aug 28, 2019

sijie deleted the cluster_get branch August 28, 2019 17:44

tisonkun pushed a commit to tisonkun/pulsar-client-go that referenced this pull request Aug 15, 2023

Add cluster commands get (streamnative/pulsarctl#4)

d84261f

Add cluster commands `get`

tisonkun pushed a commit to apache/pulsar-client-go that referenced this pull request Aug 16, 2023

Add cluster commands get (streamnative/pulsarctl#4)

64b9813

Add cluster commands `get`

	var commandPermission = "This command only admin can use."
	var commandPermission = "This command requires super-user permissions."



		var commandUsedFor = "This command is used for getting the cluster data of the specified cluster."
		var commandExample =

Add cluster commands get #4

Add cluster commands get #4

Uh oh!

Conversation

zymap commented Aug 26, 2019 • edited by sijie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sijie left a comment

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!

Uh oh!

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!

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!

zymap commented Aug 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Add cluster commands `get` #4

Add cluster commands `get` #4

zymap commented Aug 26, 2019 •

edited by sijie

Loading

zymap commented Aug 28, 2019 •

edited

Loading