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

Change AdminCLI set command to take "--vm-group" arg, and change AdminCLI user guide #1037

Merged
merged 2 commits into from
Mar 15, 2017
Merged

Change AdminCLI set command to take "--vm-group" arg, and change AdminCLI user guide #1037

merged 2 commits into from
Mar 15, 2017

Conversation

lipingxue
Copy link
Contributor

@lipingxue lipingxue commented Mar 15, 2017 •
edited by msterin

This PR includes:

  1. Change AdmingCLI command to take "--vm-group" arg instead of "--tenant" arg.
  2. Change AdminCLI user guide to make sure it up-to-date after renaming "tenant" to "vm-group".

Fixes #1026
Fixes #1035

shaominchen
shaominchen approved these changes Mar 15, 2017
View reviewed changes
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.

@msterin
Copy link
Contributor

msterin commented Mar 15, 2017

Why the CI is still failing ?

@shuklanirdesh82
Copy link
Contributor

shuklanirdesh82 commented Mar 15, 2017 •
edited

#1037 (comment)

Why the CI is still failing ?

PR is having vmdkops_admin.py with the new name and vmdkops_admin_test.py is required to be fixed as part of this PR.

Running unit tests in /tmp/vmdk_ops_unittest2428/vmdkops_admin_test.py...
............usage: vmdkops_admin_test.py set [-h] --volume VOLUME --options OPTIONS
                                 --vm-group VM_GROUP
vmdkops_admin_test.py set: error: the following arguments are required: --vm-group
E..................................

It is better to run all tests locally before pushing/generating a PR.

shuklanirdesh82
shuklanirdesh82 suggested changes Mar 15, 2017
View reviewed changes
esx_service/cli/vmdkops_admin.py
@@ -420,8 +420,8 @@ def commands():
'help': 'Volume to set options for, specified as "volume@datastore".',
'required': True
},
'--tenant': {
'help': 'Name of the tenant the volume belongs to.',
'--vm-group': {
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed offline, please run all tests make test-all with your change and provide fix for any tests referring to older name i.e. --tenant

Copy link
Contributor

@msterin msterin Mar 15, 2017 •
edited

Choose a reason for hiding this comment

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

Actually CI should be back RSN, isn't it ? So CI pass is needed as soon as maintenance is over.

Copy link
Contributor Author

@lipingxue lipingxue Mar 15, 2017 •
edited

Choose a reason for hiding this comment

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

I agree with Mark. I tried to run "make test-all" locally, it still failed at test " TestVolumeCreationFirstTime " which is a known issue. So I think there is not too much value added to run the CI test locally. If the CI test is failing with some known issue, we should disable that test temporarily.

Copy link
Contributor

Choose a reason for hiding this comment

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

TestVolumeCreationFirstTimeis related to old(er) ESX and Photon OS version only. Please check with @shuklanirdesh82 and make sure your testbed is updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Talked with @shuklanirdesh82 , upgrade the photon OS version. Then rerun test "make test-all", it failed only one test "TestConcurrency", which is a known issue.

@lipingxue
Change AdminCLI set command to take "--vm-group" arg, and change Admi…
4eeb6bd 
…nCLI user guide to make sure it is up-to-date after renaming "tenant" to "vm-group".
@lipingxue
Fix vmdkops_admin_test failure for set command.
b7afb86
@lipingxue lipingxue force-pushed the fix_user_guide_for_admincli_ls.liping branch from ad88fa6 to b7afb86 Compare March 15, 2017 21:11
@lipingxue
Copy link
Contributor Author

"make test-esx" passed.

msterin
msterin approved these changes Mar 15, 2017
View reviewed changes
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.

pending CI pass (or make test-all if CI maintenance is delayed)

@lipingxue lipingxue merged commit 2b82ad5 into master Mar 15, 2017
shuklanirdesh82 pushed a commit that referenced this pull request Mar 16, 2017
@lipingxue @shuklanirdesh82
change AdminCLI set command to take "--vm-group" arg, and change Admi…
e9bdadd 
…nCLI user guide (#1037)

* Change AdminCLI set command to take "--vm-group" arg, and change AdminCLI user guide to make sure it is up-to-date after renaming "tenant" to "vm-group".
@msterin msterin deleted the fix_user_guide_for_admincli_ls.liping branch March 22, 2017 04:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@shuklanirdesh82 shuklanirdesh82 shuklanirdesh82 requested changes

@msterin msterin msterin approved these changes

@shaominchen shaominchen shaominchen approved these changes

Assignees
No one assigned
Labels
cla-not-required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@lipingxue @msterin @shuklanirdesh82 @shaominchen @vmwclabot