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

Rename tenant to vm group.liping #1021

Merged
merged 3 commits into from
Mar 13, 2017
Merged

Conversation

lipingxue
Copy link
Contributor

This PR includes:

  1. rename "tenant" to "vm-group" in AdminCLI command
  2. rename "tenant" to "vm-group" in error message
  3. rename "tenant" to "vm-group" in related document

Most function name and DB schema remain unchanged.
Currently, "tenant" is still used in logging, I am not sure whether we should change it or not. Any comments?

@lipingxue
Copy link
Contributor Author

Test:
Basic AdminCLI command to manage vm-group passed.
Make "test-esx" also passed.

@lipingxue
Copy link
Contributor Author

@msterin @shaominchen Please review it.

@msterin
Copy link
Contributor

msterin commented Mar 10, 2017

Currently, "tenant" is still used in logging, I am not sure whether we should change it or not. Any comments?

I think yes.

Copy link
Contributor

@msterin msterin 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 to me and IMO can be merged after CI passing.
Please do reply to my comment, or stop by to discuss)


def test_tenant_create(self):
args = self.parser.parse_args('tenant create --name=tenant1 --vm-list vm1,vm2'.split())
args = self.parser.parse_args('vm-group create --name=vm-group1 --vm-list vm1,vm2'.split())
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think there was a need to change the names of groups in the test. (no need to roll back though )

self.assertEqual(args.vm_list, ['vm1', 'vm2'])

def test_tenant_create_missing_option_fails(self):
self.assert_parse_error('tenant create')
self.assert_parse_error('vm-group create')
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 it would be much better to have a global definition and use a VMGROUP name everywhere.
So this line would be self.assert_parse_error(VMGROUP + ' create')

This comment applies to ALL changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

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.

Looks good, just a few nits below. Can we file an issue to track the change for VMODL APIs? That can be fixed in future though when we really enable VMODLs.


Sample:
```
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant rm --name=tenant1 --remove-volumes
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vm-group rm --name=new-vm-group1 --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.

Nit: new-vm-group1 => vm-group1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

564df562-3d58-c99a-e76e-e8792b77ca2d photon4
564d4728-f1c7-2029-d01e-51f5e6536cd9 photon5
------------------------------------ --------
564d5849-b135-1259-cc73-d2d3aa1d9b8c photon-6
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep the name consistent? Either photon-6 and photon-7, or photon6 and photon7. Same for other occurrences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


Sample:

```shell
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant access ls --name=tenant1
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vm-group access ls --name=vm-group1
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: remove one redundant space

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant access set --name=tenant1 --datastore=datastore1 --allow-create --volume-maxsize=1000MB --volume-totalsize=2GB
tenant access set succeeded
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant access ls --name=tenant1
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vm-group access set --name=vm-group1 --datastore=datastore1 --allow-create=True --volume-maxsize=1000MB --volume-totalsize=2GB
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

```

After this point, the manually upgrade is done, and tenancy operations will succeed.

#### Case2: Has tenant configured before
#### Case2: Has vm-group configured before
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

@@ -102,56 +103,64 @@ Step 2: Move the auth-db file at /etc/vmware/vmdkops/auth-db
[root@localhost:/etc/vmware/vmdkops]mv /etc/vmware/vmdkops/auth-db /etc/vmware/vmdkops/auth-db.backup.v10.upgrade
```

Step 3: Verify “tenant ls” command, now only ```_DEFAULT``` should be listed.
Step 3: Verify “vm-group ls” command, now only ```_DEFAULT``` should be listed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

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.

None yet

4 participants