Skip to content

Fix listing management server by parameters#3840

Merged
yadvr merged 2 commits into
apache:4.13from
shapeblue:fix-listmgmtscmd
Jan 30, 2020
Merged

Fix listing management server by parameters#3840
yadvr merged 2 commits into
apache:4.13from
shapeblue:fix-listmgmtscmd

Conversation

@davidjumani
Copy link
Copy Markdown
Contributor

@davidjumani davidjumani commented Jan 27, 2020 •
edited
Loading

Description

The List Management Server api returns a list of all the management servers but fails when trying to list by id or name. This ensures that it fetches the details as per the parameters passed.
Fixes: #3833

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

How Has This Been Tested?

Verified by testing the api with and without parameters

@davidjumani davidjumani requested a review from yadvr January 27, 2020 08:30
DaanHoogland
DaanHoogland approved these changes Jan 27, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

code lgtm, description makes sense but no testing done

yadvr
yadvr approved these changes Jan 27, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@yadvr yadvr left a comment

Choose a reason for hiding this comment

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

LGTM, did not test it. A unit test would be nice to get to introduced @davidjumani usually good practice but otherwise not mandatory.

@yadvr yadvr added this to the 4.13.1.0 milestone Jan 27, 2020
@yadvr yadvr changed the base branch from master to 4.13 January 27, 2020 09:41
@yadvr
Copy link
Copy Markdown
Member

yadvr commented Jan 27, 2020

@davidjumani can you rebase against 4.13, as the issue asked for PR to be against 4.13 branch?

@shwstppr
Copy link
Copy Markdown
Contributor

shwstppr commented Jan 27, 2020

LGTM based on code changes

@yadvr yadvr closed this Jan 29, 2020
@yadvr yadvr reopened this Jan 29, 2020
@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Jan 29, 2020

@rhtyd I understand these words: "help", "hello", "thanks", "package", "test"
Test command usage: test [mgmt os] [hypervisor] [additional tests]
Mgmt OS options: ['centos6', 'centos7', 'ubuntu']
Hypervisor options: ['kvm-centos6', 'kvm-centos7', 'kvm-ubuntu', 'xenserver-71', 'xenserver-65sp1', 'xenserver-62sp1', 'vmware-65u2', 'vmware-60u2', 'vmware-55u3', 'vmware-51u1', 'vmware-50u1']
Additional tests: list of space separated tests with paths relative to the test/integration directory, for example: component/test_acl_listvm.py component/test_volumes.py
Note: when additional tests are passed, you need to specify mgmt server os and hypervisor or use the matrix command.

Blessed contributors for kicking Trillian test jobs: ['rhtyd', 'nvazquez', 'PaulAngus', 'borisstoyanov', 'DaanHoogland', 'shwstppr', 'andrijapanicsb', 'Spaceman1984', 'Pearl1594', 'davidjumani']

@apache apache deleted a comment from yadvr Jan 29, 2020
@apache apache deleted a comment from blueorangutan Jan 29, 2020
@apache apache deleted a comment from yadvr Jan 29, 2020
@apache apache deleted a comment from blueorangutan Jan 29, 2020
@apache apache deleted a comment from yadvr Jan 29, 2020
@apache apache deleted a comment from blueorangutan Jan 29, 2020
@apache apache deleted a comment from blueorangutan Jan 29, 2020
@apache apache deleted a comment from yadvr Jan 29, 2020
@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Jan 29, 2020

Trillian test result (tid-835)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 41805 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3840-t835-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 77 look OK, 0 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File

yadvr
yadvr requested changes Jan 30, 2020
View reviewed changes
Comment thread server/src/main/java/com/cloud/api/query/QueryManagerImpl.java Outdated
Pair<List<ManagementServerHostVO>, Integer> result = listManagementServersInternal(cmd);
List<ManagementServerResponse> hostResponses = new ArrayList<>();

for(ManagementServerHostVO host : result.first()) {
Copy link
Copy Markdown
Member

@yadvr yadvr Jan 30, 2020

Choose a reason for hiding this comment

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

Add space after for and ifs

@yadvr yadvr merged commit 7a25e40 into apache:4.13 Jan 30, 2020
ustcweizhou pushed a commit to ustcweizhou/cloudstack that referenced this pull request Feb 28, 2020
@davidjumani @ustcweizhou
api: allow listing management server by id and name (apache#3840)
4ec75c1 
The List Management Server api returns a list of all the management servers but fails when trying to list by id or name. This ensures that it fetches the details as per the parameters passed.
Fixes: apache#3833
@davidjumani davidjumani deleted the fix-listmgmtscmd branch October 27, 2020 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yadvr yadvr yadvr approved these changes

@shwstppr shwstppr shwstppr approved these changes

@DaanHoogland DaanHoogland DaanHoogland approved these changes

Assignees

No one assigned

Labels

component:api component:management-server type:bug

Projects

None yet

Milestone

4.13.1.0

Development

Successfully merging this pull request may close these issues.

6 participants

@davidjumani @yadvr @shwstppr @blueorangutan @DaanHoogland @svenvogel