Skip to content
This repository has been archived by the owner on Nov 9, 2020. It is now read-only.

Test to verify add, remove and replace of a vm from the _DEFAULT tenant #1283

Conversation

ashahi1
Copy link
Contributor

@ashahi1 ashahi1 commented May 22, 2017

Test Steps:

  1. Add a vm to the _DEFAULT tenant
  2. Remove a vm from the _DEFAULT tenant
  3. Replace a vm from the _DEFAULT tenant

Please refer Issue #1125

Output:

----TestAddRmvReplaceVmToTenant  Starts-------
2017/05/22 20:03:36 Adding VM [ubuntu-VM0.3] to a vmgroup [_DEFAULT] on esx [10.161.233.63]
2017/05/22 20:03:37 Not able to add a vm to the _DEFAULT tenant
2017/05/22 20:03:37 Removing VM [ubuntu-VM0.3] from a vmgroup [_DEFAULT] on esx [10.161.233.63]
2017/05/22 20:03:37 Not able to remove a vm from the _DEFAULT tenant
2017/05/22 20:03:37 Replacing VM [ubuntu-VM0.3] from a vmgroup [_DEFAULT] on esx [10.161.233.63]
--- PASS

Copy link
Contributor

@pshahzeb pshahzeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
Some changes for code placement, logging


// vmdkops_admin volume
vmdkopsAdminVolume = vmdkopsAdmin + "volume "
VmdkopsAdminVolume = VmdkopsAdmin + "volume "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these consts are local. you can define more consts that in turn use these...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. These constants are intentionally kept starting with small case, because they are not meant to be exported.

expctdErr = "This feature is not supported for vmgroup _DEFAULT."
)

type AddRmvReplaceTestSuite struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DefaultTenantTestSuite may be? or anything better than. this name doesn't tell much about the test suite

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. The struct name should be a noun.

// 2. Remove a vm from the _DEFAULT tenant
// 3. Replace a vm from the _DEFAULT tenant
func (s *AddRmvReplaceTestSuite) TestAddRmvReplaceVmToTenant(c *C) {
log.Printf("----TestAddRmvReplaceVmToTenant Starts------- ")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we can follow a logging standard here.
Something like this
log.Printf("START: basic-test.TestVolumeLifecycle")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. Let's follow a convention for tracing logs.

// 3. Replace a vm from the _DEFAULT tenant
func (s *AddRmvReplaceTestSuite) TestAddRmvReplaceVmToTenant(c *C) {
log.Printf("----TestAddRmvReplaceVmToTenant Starts------- ")
out, err := vmgroup.AddVmToVmgroup(s.esxName, "_DEFAULT", s.hostName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: put _DEFAULT in constants package

c.Assert(err, IsNil, Commentf(misc.FormatOutput(out)))
c.Assert(strings.TrimRight(string(out), "\n"), Equals, expctdErr, Commentf("ERROR: "+
"Able to add vm to the _DEFAULT tenant"))
log.Printf("Not able to add a vm to the _DEFAULT tenant")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need this printfs. Assert passed itself means that we weren't able to add vm to default tenant( and that is the intention of the test). Trying to keep logs clean.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

// DeleteVmgroup method deletes a vmgroup and removes its volumes as well
func DeleteVmgroup(ip, name string) ([]byte, error) {
log.Printf("Deleting a vmgroup [%s] on esx [%s]\n", name, ip)
return ssh.InvokeCommand(ip, admincli.VmdkopsAdmin+" vmgroup rm --name=" + name + " --remove-volumes")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these vmgroup operations (rm, add, create) can be declared as constants

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

out, err = vmgroup.RemoveVmFromVmgroup(s.esxName, "_DEFAULT", s.hostName)
c.Assert(err, IsNil, Commentf(misc.FormatOutput(out)))
c.Assert(strings.TrimRight(string(out), "\n"), Equals, expctdErr, Commentf("ERROR: "+
"Able to remove vm from the _DEFAULT tenant"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Able to remove vm from the _DEFAULT tenant" sounds positive.
Can we have a negative message since it would be printed in case when the assert failed i.e. you got something different from the expected error defined by var "expctdErr"

Copy link
Contributor

@shaominchen shaominchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rename "addremoverereplacevmtodefaulttenant_test" to something like "default_tenant_test"?


// vmdkops_admin volume
vmdkopsAdminVolume = vmdkopsAdmin + "volume "
VmdkopsAdminVolume = VmdkopsAdmin + "volume "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. These constants are intentionally kept starting with small case, because they are not meant to be exported.

@@ -0,0 +1,67 @@
//
// http://www.apache.org/licenses/LICENSE-2.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have VMware copyright info here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls. change file name to a small one like defaultvmgroup.go, #1262 is also adding tests for the default tenant and may make it into this file vs. a file for each test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah agree .. one suggestion though defaultvmgroup_test.go instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have re-named it as defaultvmgroup_test.go

// limitations under the License.

// This test tries to add, remove and replace vm to the _DEFAULT tenant.
// Expected behavior is that add/rmv/replace vms for _DEFAULT vmgroup should fail.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: rmv => rm

)

const (
expctdErr = "This feature is not supported for vmgroup _DEFAULT."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Let's avoid using uncommon abbreviations - "expctd" looks weird. We can rename this to something like ErrorMsg.

expctdErr = "This feature is not supported for vmgroup _DEFAULT."
)

type AddRmvReplaceTestSuite struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. The struct name should be a noun.

// 2. Remove a vm from the _DEFAULT tenant
// 3. Replace a vm from the _DEFAULT tenant
func (s *AddRmvReplaceTestSuite) TestAddRmvReplaceVmToTenant(c *C) {
log.Printf("----TestAddRmvReplaceVmToTenant Starts------- ")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. Let's follow a convention for tracing logs.

c.Assert(err, IsNil, Commentf(misc.FormatOutput(out)))
c.Assert(strings.TrimRight(string(out), "\n"), Equals, expctdErr, Commentf("ERROR: "+
"Able to add vm to the _DEFAULT tenant"))
log.Printf("Not able to add a vm to the _DEFAULT tenant")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

// DeleteVmgroup method deletes a vmgroup and removes its volumes as well
func DeleteVmgroup(ip, name string) ([]byte, error) {
log.Printf("Deleting a vmgroup [%s] on esx [%s]\n", name, ip)
return ssh.InvokeCommand(ip, admincli.VmdkopsAdmin+" vmgroup rm --name=" + name + " --remove-volumes")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

// 1. Add a vm to the _DEFAULT tenant
// 2. Remove a vm from the _DEFAULT tenant
// 3. Replace a vm from the _DEFAULT tenant
func (s *AddRmvReplaceTestSuite) TestAddRmvReplaceVmToTenant(c *C) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to rename this to TestModifyDefaultTenantVMs. Basically the intention is to verify whether modifying default tenant VMs is allowed or not.


// ListVolumes referring to vmdkops_admin volume ls
ListVolumes = vmdkopsAdminVolume + "ls "
ListVolumes = VmdkopsAdminVolume + "ls "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to revert this back.

)

const (
expctdErr = "This feature is not supported for vmgroup _DEFAULT."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to move such constant to dedicated constant file

)

type AddRmvReplaceTestSuite struct {
esxName string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

esxName => esxIP .. we are exporting IPs not Names.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take care about the refactoring if you find such kind of variables into other files.


type AddRmvReplaceTestSuite struct {
esxName string
hostName string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

}

func (s *AddRmvReplaceTestSuite) SetUpTest(c *C) {
s.hostName = govc.RetrieveVMNameFromIP(os.Getenv("VM1"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's use VM2 .. until we fix photon related issue to retrieve vmname from IP using govc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes...good one!

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

// This util is going to hold various helper methods related to vmgroup to be consumed by testcases.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"is going to hold" => holds

// vmgroup creation, deletion is supported as of now.


package vmgroup
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vmgroup creation/deletion should be considered as part of AdminCLI.

I would suggest to place at /utils/admincli

Following is the highlevel structure

tests
   |-e2e
   |-consts
   |-utils
      |-admincli ( if you want you may create subfolder here)
      |-dockercli
      |-govc
      |-misc
      |- ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmmm...ok!



// CreateVmgroup method is going to create a vmgroup and adds vm to it
func CreateVmgroup(ip, name string, vmName string) ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ip, name string, vmName string => ip, name, vmName string .. to keep it consistent

// CreateVmgroup method is going to create a vmgroup and adds vm to it
func CreateVmgroup(ip, name string, vmName string) ([]byte, error) {
log.Printf("Creating a vmgroup [%s] on esx [%s]\n", name, ip)
return ssh.InvokeCommand(ip, admincli.VmdkopsAdmin+" vmgroup create --name="+name + " --default-datastore=\"_VM_DS\" --vm-list=" + vmName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to keep it consistent ... command should be placed at const file

return ssh.InvokeCommand(ip, admincli.VmdkopsAdmin+" vmgroup vm rm --name=" + name + " --vm-list=" + vmName )
}


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: please remove extra new lines

AddVmToVmgroup = vmdkopsAdmin + "vmgroup vm add "

// remove a vm from vmgroup
RemoveVmFrmVmgroup = vmdkopsAdmin + "vmgroup vm rm "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: let's avoid uncommon abbreviations: "Frm" => "From"

c.Assert(err, IsNil, Commentf(misc.FormatOutput(out)))
c.Assert(strings.TrimRight(string(out), "\n"), Equals, ErrorMsg, Commentf("Unexpected Behavior: "+
"We are able to replace vm from the _DEFAULT tenant which is NOT allowed as per current spec."))
log.Printf("----Test Ends----")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep the log style consistent:

log.Printf("END: defaultvmgroup.TestModifyDefaultTenantVMs")

// This util holds various helper methods related to vmgroup to be consumed by testcases.
// vmgroup creation, deletion or adding, removing and replacing vm from vmgroup is supported currently.

package vmgroup
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a separate package for this util? I think we should keep it in admincli package, so that we can use the functions in this util like this: admincli.CreateVmgroup(), which makes more sense than vmgroup.CreateVmgroup()

CreateVmgroup = vmdkopsAdmin + "vmgroup create "

// remove vmgroup
RemoveVmgroup = vmdkopsAdmin + "vmgroup rm "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add remaining two constants as well? for list and update

c.Assert(strings.TrimRight(string(out), "\n"), Equals, ErrorMsg, Commentf("Unexpected Behavior: "+
"We are able to add vm to the _DEFAULT tenant which is NOT allowed as per current spec."))

out, err = vmgroup.RemoveVmFromVmgroup(s.esxIP, admincli.DefaultVmgroup, s.hostName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is better to break this test into 3 test methods ... incase of failure for first wont run rest steps.

@@ -25,4 +25,28 @@ const (

// ListVolumes referring to vmdkops_admin volume ls
ListVolumes = vmdkopsAdminVolume + "ls "

// CreateVmgroup referring to create vmgroup
CreateVmgroup = vmdkopsAdmin + "vmgroup create "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add remaining two constants as well? for list and update

c.Assert(strings.TrimRight(string(out), "\n"), Equals, ErrorMsg, Commentf("Unexpected Behavior: "+
"We are able to add vm to the _DEFAULT tenant which is NOT allowed as per current spec."))

out, err = admincli.RemoveVmFromVmgroup(s.esxIP, con.DefaultVmgroup, s.hostName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is better to break this test into 3 test methods ... incase of failure for first wont run rest steps.

Copy link
Contributor

@pshahzeb pshahzeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just one comment about package alias.

// 3. Replace a vm from the _DEFAULT tenant
func (s *DefaultVmGroupTestSuite) TestModifyDefaultTenantVMs(c *C) {
log.Printf("START: defaultvmgroup.TestModifyDefaultTenantVMs")
out, err := admincli.AddVmToVmgroup(s.esxIP, con.DefaultVmgroup, s.hostName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the alias con is because of the conflicting package name "admincli". Are we opting for aliases in such cases? //CC @shuklanirdesh82 @shaominchen

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the two conflicting package names:
github.com/vmware/docker-volume-vsphere/tests/constants/admincli
github.com/vmware/docker-volume-vsphere/tests/utils/admincli

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are conflicts, we have to use alias in such cases. However, for this minimal project, we are running into package conflicts with our own packages! This is an obvious evidence that we are not managing packages properly. I think we should move all existing constant files to the same package under utils. Let's revisit this later as a refactoring issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with this respective, we should rename package by suffix it with "const" ..

e.g. github.com/vmware/docker-volume-vsphere/tests/constants/admincliconst

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one option, but we might have other better options to avoid so many folders/packages just for constants. Let's think about this and revisit this part if any ideas coming to mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have re-named the package to github.com/vmware/docker-volume-vsphere/tests/constants/admincliconst

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's hold on this change for now and keep the alias. Later we can have a separate refactoring change to address this package name conflict issue.

Copy link
Contributor

@shaominchen shaominchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please address the remaining nits and check CI failures.

@@ -52,3 +52,15 @@ func ReplaceVmFromVmgroup(ip, name, vmName string) ([]byte, error) {
log.Printf("Replacing VM [%s] from a vmgroup [%s] on esx [%s] \n", vmName, name, ip)
return ssh.InvokeCommand(ip, admincli.ReplaceVmFrmVmgroup+"--name="+name+" --vm-list="+vmName)
}

// Initialize the (local) Single Node Config DB
func ConfigInit(ip string) ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should improve this function to support both SingleNode and MultiNode. But I'm fine to take care of this whenever we need it afterwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I thought about this while writing SingleNode util but since we do not need it for this test I didn't add it. I can address this later on.

Copy link
Contributor

@shuklanirdesh82 shuklanirdesh82 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me .. just a minor comment.


// Test step:
// Add a vm to the _DEFAULT Vmgroup
func (s *DefaultVmGroupTestSuite) TestAddVmToDefaultTenant(c *C) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to maintain consistency Vm should be VM e.g. TestAddVMToDefaultTenant .. need to replace it everywhere.

Copy link
Contributor

@pshahzeb pshahzeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Contingent to CI pass.

Copy link
Contributor

@shuklanirdesh82 shuklanirdesh82 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashahi1 let's unblock this test automation as the reported issue has root caused.

If CI passes after addressing comment, we should proceed with checkin.

hostName string
}

func (s *DefaultVMGroupTestSuite) SetUpTest(c *C) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while talking with @pshahzeb came to know that we missed this line. This should be test suite level.

}

/*
func (s *DefaultVMGroupTestSuite) TearDownTest(c *C) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above .. should be test suite level.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes...I tested it locally yesterday and it was passing. Let me make a fresh push with these changes and check if CI passes. We can add another test that does config init and config rm multiple times and then check the status of vmdk-opsd.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good. Please make sure to raise a tracking issue to add new steps to cover above steps.

@shuklanirdesh82
Copy link
Contributor

@ashahi1, Please rebase your brach with master and run locally before triggering CI run.

From your PR's current CI run: https://ci.vmware.run/vmware/docker-volume-vsphere/258

=> Running target e2e-dkrVolDriver-test Thu May 25 17:28:11 UTC 2017

# github.com/vmware/docker-volume-vsphere/tests/utils/admincli
../tests/utils/admincli/vmgroupmgmt.go:29: cannot use string as type []byte in return argument
../tests/utils/admincli/vmgroupmgmt.go:35: cannot use string as type []byte in return argument
../tests/utils/admincli/vmgroupmgmt.go:41: cannot use string as type []byte in return argument
../tests/utils/admincli/vmgroupmgmt.go:47: cannot use string as type []byte in return argument
../tests/utils/admincli/vmgroupmgmt.go:53: cannot use string as type []byte in return argument
../tests/utils/admincli/vmgroupmgmt.go:59: cannot use string as type []byte in return argument
../tests/utils/admincli/vmgroupmgmt.go:65: cannot use string as type []byte in return argument
FAIL	github.com/vmware/docker-volume-vsphere/tests/e2e [build failed]
make[1]: *** [e2e-dkrVolDriver-test] Error 2
make: *** [e2e-dkrVolDriver-test] Error 2

P.S.: We should start taking CI failure seriously by avoid such obvious failures.

/CC @tusharnt

@ashahi1 ashahi1 force-pushed the verifyAddingVmToDefaultTenant.ashahi1 branch from 419f380 to 9127201 Compare May 25, 2017 18:56
// ListVMgroups referring to vmdkops_admin vmgroups ls
ListVMgroups = vmdkopsAdmin + "vmgroup ls "

// UpdateVMgroup referring to updating a vmgroup
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For all the commands that take a "--name" arg - especially for the vmgroup commands please add that (--name) to the constants above. The users of these constants need less string additions in the calling code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@govint govint mentioned this pull request May 26, 2017
func (s *DefaultVMGroupTestSuite) SetUpSuite(c *C) {
s.hostName = govc.RetrieveVMNameFromIP(os.Getenv("VM2"))
s.esxIP = os.Getenv("ESX")
out, err := admincli.ConfigInit(os.Getenv("ESX"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls. get all values from the inputparams pkg. Thats the single place where env/govc should be used to get values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be addressed in #1305

@shuklanirdesh82
Copy link
Contributor

@ashahi1 Please squash all commits ... there should be atleast 1 commit.

@ashahi1 ashahi1 force-pushed the verifyAddingVmToDefaultTenant.ashahi1 branch from 910b7ba to 95595e1 Compare May 26, 2017 23:32
@ashahi1 ashahi1 force-pushed the verifyAddingVmToDefaultTenant.ashahi1 branch from 95595e1 to 697b1d0 Compare May 26, 2017 23:33
@shuklanirdesh82 shuklanirdesh82 merged commit e410302 into vmware-archive:master May 27, 2017
@ashahi1 ashahi1 deleted the verifyAddingVmToDefaultTenant.ashahi1 branch June 8, 2017 00:11
@ashahi1
Copy link
Contributor Author

ashahi1 commented Jun 23, 2017

fix for case 2 #1261

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants