New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Tags Categories cmd available #1150

Merged

dougm merged 8 commits into vmware:master from jiatongw:master

Jun 13, 2018

jiatongw commented Jun 5, 2018 •

edited

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

The tags management issue can be found here #1139

vmwclabot added the cla-not-required label

Author

jiatongw commented Jun 5, 2018

The tags_helper/ are from VIC project

jiatongw force-pushed the master branch 3 times, most recently from d55c985 to f772395 Compare

June 6, 2018 22:03

dougm reviewed

View reviewed changes

Member

dougm left a comment

Looking good @jiatongw
I haven't fully reviewed yet, but here are some initial comments. And a few general comments:

New files need the license header
Error messages should not be written to stdout (via fmt.Printf) - just return the error and add details using fmt.Errorf if needed.
I think tags.categories should be renamed tags.category
We need to think about a different location for tags_helper - we want this client library to be usable outside of govc

govc/main.go Outdated

@@ @@ -20,6 +20,8 @@ import ( @@
               	"os"
               	"github.com/vmware/govmomi/govc/cli"
+              	_ "github.com/vmware/govmomi/govc/tags/categories"
+              	_ "github.com/vmware/govmomi/govc/tags/tags_helper"

Member

dougm Jun 6, 2018

No need to import tags_helper here.

govc/tags/categories/categories_create.go Outdated

+              }
+              func (cmd *create) Usage() string {
+              	return `

Member

dougm Jun 6, 2018

Include a description on the first line of Usage here.

govc/tags/categories/categories_create.go Outdated

+              func (cmd *create) Usage() string {
+              	return `
+              	Examples:

Member

dougm Jun 6, 2018

Indentation needs to follow the other examples. That is:

No spaces for before Examples:
Two spaces before govc ...

govc/tags/categories/categories_create.go Outdated

+              		return flag.ErrHelp
+              	}
+              	name := f.Arg(0)

Member

dougm Jun 7, 2018

I think name should be the only required argument. Flags for the others:
-description, -type and -multi. The Register method can be used to add the flags.

govc/tags/categories/categories_create.go Outdated

+              		id, err := c.CreateCategoryIfNotExist(ctx, name, description, categoryType, value)
+              		if err != nil {
+              			fmt.Printf(err.Error())

Member

dougm Jun 7, 2018

You can remove this line, the returned error below will be printed by the cli framework.

govc/tags/categories/categories_ls.go Outdated

+              		return err
+              	}
+              	govcUrl := "https://" + os.Getenv("GOVC_URL")

Member

dougm Jun 7, 2018

The GOVC_URL env var may not be set. Instead use vc.URL() and cmd.Userinfo()

govc/tags/categories/categories_ls.go Outdated

+              		return err
+              	}
+              	// SSO admin server has its own session manager, so the govc persisted session cookies cannot

Member

dougm Jun 7, 2018

You can remove the SSO related code here (lines 61-87).

govc/tags/categories/categories_ls.go Outdated

+              		}
+              	}
+              	if err = c.Login(context.TODO()); err != nil {

Member

dougm Jun 7, 2018

Use the ctx var here instead of context.TODO()

govc/tags/categories/categories_ls.go Outdated

+              	return withClient(ctx, cmd.ClientFlag, func(c *tags.RestClient) error {
+              		categories, err := c.ListCategories(ctx)
+              		if err != nil {
+              			fmt.Printf(err.Error())

Member

dougm Jun 7, 2018

Remove this line.

govc/tags/categories/categories_ls.go Outdated

+              			return err
+              		}
+              		fmt.Println(categories)

Member

dougm Jun 7, 2018

Here you can use cmd.WriteResult instead, to support the -json and -dump flags. For an example, see:

govmomi/govc/object/find.go

Lines 176 to 187 in 7736cdc

    
           type findResult []string 
        
           func (r findResult) Write(w io.Writer) error { 
        
           	for i := range r { 
        
           		fmt.Fprintln(w, r[i]) 
        
           	} 
        
           	return nil 
        
           } 
        
           func (r findResult) Dump() interface{} { 
        
           	return []string(r) 
        
           }

dougm reviewed

View reviewed changes

govc/tags/tags_helper/categories.go Outdated

+              // See the License for the specific language governing permissions and
+              // limitations under the License.
+              package tags

Member

dougm Jun 8, 2018

We need this package to be outside of the govc directory, as it will be consumed outside of govc. The rest APIs are referred to as vAPI, so let's move the govc/tags/tags_helper directory to vapi/tags at the top-level of the repo.

Jiatong Wang added 5 commits

June 8, 2018 21:28


          categories cmd full version


          remove errors pkg

f4f8ae6


          fixed printf bugs

6b1ea2b


          tags category apis

4d8b799


          delete useless file

ec5f540

dougm reviewed

View reviewed changes

Member

dougm left a comment

@jiatongw all the files named .DS_Store need to be removed from this PR. You can add the file name to your ~/.gitignore to avoid adding them in the future.

govc/main.go Outdated

@@ @@ -72,6 +72,7 @@ import ( @@
               	_ "github.com/vmware/govmomi/govc/session"
               	_ "github.com/vmware/govmomi/govc/sso/service"
               	_ "github.com/vmware/govmomi/govc/sso/user"
+              	_ "github.com/vmware/govmomi/govc/tags/categories"

Member

dougm Jun 11, 2018

The directory should also be renamed to category

govc/tags/categories/create.go Outdated

+              limitations under the License.
+              */
+              package tags

Member

dougm Jun 11, 2018

The directory and package should be named category.

govc/tags/categories/create.go Outdated

+              func (cmd *create) Register(ctx context.Context, f *flag.FlagSet) {
+              	cmd.ClientFlag, ctx = flags.NewClientFlag(ctx)
+              	cmd.ClientFlag.Register(ctx, f)
+              	f.StringVar(&cmd.description, "d", "", "description of category")

Member

dougm Jun 11, 2018

The usage strings should all start with an upper case letter, Description ...

govc/tags/categories/get.go Outdated

+              	name := f.Arg(0)
+              	return withClient(ctx, cmd.ClientFlag, func(c *tags.RestClient) error {
+              		categoriesName, err := c.GetCategoriesByName(context.Background(), name)

Member

dougm Jun 11, 2018

Use the ctx var here instead of context.Background()

govc/tags/categories/ls.go Outdated

+              	if err != nil {
+              		return err
+              	}
+              	usrDecode, err := url.QueryUnescape(cmd.Userinfo().String())

Member

dougm Jun 11, 2018

Instead, you can just use:

tagsURL := vc.URL()
tagsURL.User = cmd.Userinfo()

govc/tags/categories/ls.go Outdated

+              		return err
+              	}
+              	c := tags.NewClient(URL, true, "")

Member

dougm Jun 11, 2018

Instead of hardcoding true for the insecure param, you can use !cmd.IsSecure()

govc/tags/categories/ls.go Outdated

+              		return err
+              	}
+              	token := os.Getenv("GOVC_LOGIN_TOKEN")

Member

dougm Jun 11, 2018

The sso related code here through line 108 can be removed.

govc/tags/categories/ls.go Outdated

+              	if err = c.Login(ctx); err != nil {
+              		return err
+              	}

Member

dougm Jun 11, 2018

Add a TODO: here, we should have a defer c.Logout() method here. Otherwise the commands will increase active sessions, for which vCenter has a maximum.

govc/tags/categories/create.go Outdated

+              	return nil
+              }
+              func (cmd *create) Usage() string {

Member

dougm Jun 11, 2018

This method should be named Description. The Usage method just returns the command arguments, in this case NAME. Have a look at the output of tags.category.create -h to see the formatted help text these methods contribute to.


          Add Description() method and and rename category package and files

6211bca

Rename categories folder to category, as well as the package name. Add Description() and Usage() methods. Add Logout() in withClient() method to release resource. Format outputs.

jiatongw force-pushed the master branch 3 times, most recently from 90986c4 to 2508c9e Compare

June 12, 2018 21:54


          Add Logout() method to RestCilent type as a receiver function

44ba72e

jiatongw force-pushed the master branch from 5264799 to 44ba72e Compare

June 12, 2018 22:02

Author

jiatongw commented Jun 12, 2018

@dougm Updated PR starts from 6211bca


          Delete .DS_Store files

f033846

jiatongw commented

View reviewed changes

govc/tags/category/ls.go

+              	}
+              	tagsURL := vc.URL()
+              	tagsURL.User = cmd.Userinfo()

Author

jiatongw Jun 12, 2018

These two lines (vc.URL()) work in version 6.7, but will fail in version 6.5

dougm approved these changes

View reviewed changes

Member

dougm left a comment

Nice work @jiatongw , lgtm

dougm merged commit 1ddfb01 into vmware:master

This was referenced Jul 16, 2018

SSO support for vAPI #1179

Closed

Finalize tags API for 0.19 release #1180

Closed

dougm added this to the 2018.09 milestone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment