Complete tags management APIs #1162

jiatongw · 2018-06-15T23:06:46Z

Tags management in govc has 3 steps: 1. Creating tag category. 2. Creating tag. 3.Attaching created tag. This PR is related with the second and last steps.

The tags management issue can be found here #1139

dougm

Looking good @jiatongw
Note that several of my comments apply to all commands.

dougm · 2018-06-19T17:51:37Z

govc/tags/association/attach.go

+}
+
+func (cmd *attach) Description() string {
+	return ` Attach tag to object.


Leading space should be removed.

dougm · 2018-06-19T17:53:32Z

govc/tags/association/attach.go

+				return "", "", fmt.Errorf("%s not found", managedObj)
+			case 1:
+				ref = l[0].Object.Reference()
+				s := strings.Split(ref.String(), ":")


You can use ref.Type and ref.Value here, rather than splitting the String(). This function could also just return types.ManagedObjectReference instead of the 2 strings.

dougm · 2018-06-19T17:56:26Z

govc/tags/association/attach.go

+				return "", "", flag.ErrHelp
+			}
+		} else {
+			s := strings.Split(managedObj, ":")


Same comment as above, no need to Split.

dougm · 2018-06-19T17:58:13Z

govc/tags/association/attach.go

+)
+
+type attach struct {
+	*flags.ClientFlag


You can remove this line, since DatacenterFlag already embeds a ClientFlag.

dougm · 2018-06-19T17:59:00Z

govc/tags/association/attach.go

+}
+
+func (cmd *attach) Register(ctx context.Context, f *flag.FlagSet) {
+	cmd.ClientFlag, ctx = flags.NewClientFlag(ctx)


And remove these 2 ClientFlag lines, as the DatacenterFlag.Register below takes care of this.

dougm · 2018-06-19T18:04:52Z

govc/tags/tags/get.go

+
+func (r getTagName) Write(w io.Writer) error {
+	for i := range r {
+		fmt.Fprintln(w, r[i])


Since tags.Tag is a structure rather than just a string, the Write command should use TabWriter to format the output.

Example:

govmomi/govc/role/ls.go

Lines 69 to 75 in b325163

tw := tabwriter.NewWriter(w, 2, 0, 2, ' ', 0)

for _, role := range rl {

fmt.Fprintf(tw, "%s\t%s\n", role.Name, role.Info.GetDescription().Summary)

}

return tw.Flush()

dougm · 2018-06-19T18:09:46Z

vapi/tags/tag_association.go

+	return &spec
+}
+
+func (c *RestClient) AttachTagToObject(ctx context.Context, tagID string, objID string, objType string) error {


For these methods, instead of tagID and objID as string inputs, can we use 1 input types.ManagedObjectReference instead?

dougm · 2018-06-19T18:11:48Z

vapi/tags/tag_association.go

+	_, _, status, err := c.call(ctx, "POST", fmt.Sprintf("%s?~action=attach", TagAssociationURL), *spec, nil)
+
+	if status != http.StatusOK || err != nil {
+		return fmt.Errorf("Attach tag failed with status code: %d, error message: %s", status, err)


"Error strings should not be capitalized ..." https://github.com/golang/go/wiki/CodeReviewComments#error-strings

dougm · 2018-06-19T18:14:31Z

vapi/tags/tags.go

+}
+
+func (c *RestClient) CreateTag(ctx context.Context, spec *TagCreateSpec) (*string, error) {
+	stream, _, status, err := c.call(ctx, "POST", TagURL, spec, nil)


We could use the http.MethodPost constant instead of literal "POST", same for the other http methods.

dougm · 2018-06-19T18:15:55Z

vapi/tags/tags.go

+	if err != nil {
+		return err
+	}
+	if objs != nil && len(objs) > 0 {


No need to check objs != nil here, len() will return 0 if objs is nil.

jiatongw · 2018-06-20T23:19:10Z

govc/tags/tags/get.go

+			if err != nil {
+				return err
+			}
+			fmt.Println(*tag)


The TabWriter function works well with []tags.Tag type. But cannot use to type *tag. (the Writer function inside there is a loop, which cannot range over that type..) Any suggestions? @dougm

No need to use tabwriter when there would only be 1 row, which seems to be the case here. I think you can just do something space delimited like: fmt.Println(tag.ID, tag.Name, tag.Description, tag.OtherField)

Add tags/association/ and tags/tags/ folder. Add import packages in main.go. Add two tags helper files to vapi/.

…ant code Use tabWriter to output a struct, move govc/tags/tags/ files to govc/tags/, remove ClientFlags register and Process() function in which DatecenterFlag is set, use http.Method to replace "GET", "POST", "DELETE", "PATCH" in vapi/tags/ files.

dougm · 2018-06-21T22:21:38Z

govc/tags/association/attach.go

+	return `Attach tag to object.
+
+Examples:
+  govc tags.attach ID PATH`


Can you also use example values for the Examples sections? Instead of PATH for example, /dc1/host/cluster1/hostname

dougm · 2018-06-21T22:26:39Z

govc/tags/create.go

+}
+
+func (cmd *create) Description() string {
+	return ` Create tag. This command will output the ID you just created.


Remove leading space. Add newline after summary.

dougm · 2018-06-21T22:29:02Z

govc/tags/get.go

+	tw := tabwriter.NewWriter(w, 2, 0, 2, ' ', 0)
+
+	for _, item := range t {
+		fmt.Fprintf(tw, "Name: %s\nID: %s\nDescription: %s\nCategoryID: %s\nUsedBy: %s\n",


There needs to be tab delimiters (\t) for tabwriter. Have a look at the existing govc/*/info.go commands. Speaking of which, this command should probably named tags.info rather than tags.get. You'll see we have many *.info commands that are similar, but we have not commands named get.

dougm · 2018-06-21T22:47:57Z

govc/tags/get.go

+			if err != nil {
+				return err
+			}
+			fmt.Printf("Name: %s\nID: %s\nDescription: %s\nCategoryID: %s\nUsedBy: %s\n",


You can use the same output formatter in this case. For example the code could be changed to only call WriteResult in both cases:

var result []tags.Tag if cmd.name == "" { tag, err := c.GetTag(ctx, id) if err != nil { return err } result = append(result, tag) } else { tagSlice, err := c.GetTagByNameForCategory(ctx, cmd.name, id) if err != nil { return err } result = getTagName(tagSlice) } return cmd.WriteResult(result)

jiatongw · 2018-06-22T21:16:51Z

govc/tags/info.go

+		fmt.Fprintf(tw, "  ID:\t%s\n", item.ID)
+		fmt.Fprintf(tw, "  Description:\t%s\n", item.Description)
+		fmt.Fprintf(tw, "  CategoryID:\t%s\n", item.CategoryID)
+		fmt.Fprintf(tw, "  UsedBy: %s\n", item.UsedBy)
 	}


Separate fmt.Fprintf() for readable and matching other govc/*/info.go code style

… tabwriter

dougm

lgtm, thanks @jiatongw

vmwclabot added the cla-not-required label Jun 15, 2018

jiatongw changed the title ~~Completed tags management APIs~~ Complete tags management APIs Jun 15, 2018

dougm reviewed Jun 19, 2018

View reviewed changes

jiatongw commented Jun 20, 2018

View reviewed changes

Jiatong Wang added 2 commits June 21, 2018 14:46

Completed tags management APIs

144cb69

Add tags/association/ and tags/tags/ folder. Add import packages in main.go. Add two tags helper files to vapi/.

jiatongw force-pushed the tags branch from 287bbc2 to eab3e3d Compare June 21, 2018 21:47

dougm reviewed Jun 21, 2018

View reviewed changes

jiatongw force-pushed the tags branch from 0e2bbf6 to 9718cd0 Compare June 22, 2018 18:40

jiatongw commented Jun 22, 2018

View reviewed changes

Change tags.get command to tags.info, format and optimize output with…

1426108

… tabwriter

jiatongw force-pushed the tags branch from 9718cd0 to 1426108 Compare June 22, 2018 22:01

dougm approved these changes Jun 25, 2018

View reviewed changes

dougm merged commit 919b728 into vmware:master Jun 25, 2018

jiatongw deleted the tags branch June 25, 2018 17:59

dougm added this to the 2018.09 milestone Sep 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Complete tags management APIs #1162

Complete tags management APIs #1162

jiatongw commented Jun 15, 2018 •

edited

Loading

dougm left a comment

dougm Jun 19, 2018

dougm Jun 19, 2018

dougm Jun 19, 2018

dougm Jun 19, 2018

dougm Jun 19, 2018

dougm Jun 19, 2018

dougm Jun 19, 2018

dougm Jun 19, 2018

dougm Jun 19, 2018

dougm Jun 19, 2018

dougm Jun 19, 2018

jiatongw Jun 20, 2018

dougm Jun 21, 2018

dougm Jun 21, 2018

dougm Jun 21, 2018

dougm Jun 21, 2018

dougm Jun 21, 2018

jiatongw Jun 22, 2018

jiatongw Jun 22, 2018 •

edited

Loading

dougm left a comment

	tw := tabwriter.NewWriter(w, 2, 0, 2, ' ', 0)

	for _, role := range rl {
	fmt.Fprintf(tw, "%s\t%s\n", role.Name, role.Info.GetDescription().Summary)
	}

	return tw.Flush()

Complete tags management APIs #1162

Complete tags management APIs #1162

Conversation

jiatongw commented Jun 15, 2018 • edited Loading

dougm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiatongw Jun 22, 2018 • edited Loading

Choose a reason for hiding this comment

dougm left a comment

Choose a reason for hiding this comment

jiatongw commented Jun 15, 2018 •

edited

Loading

jiatongw Jun 22, 2018 •

edited

Loading