Fix: Listing projects comprising of only the user's on listAll=true by Pearl1594 · Pull Request #4469 · apache/cloudstack

Pearl1594 · 2020-11-13T08:46:13Z

Description

A few changes made to server/src/main/java/com/cloud/api/query/QueryManagerImpl.java were reverted by commit: fc05d3168f14584431a1a6b3c1f88cf04999cfaa
Fix a check for parameters passed

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)

Pearl1594 · 2020-11-13T08:46:54Z

@blueorangutan package

blueorangutan · 2020-11-13T08:47:29Z

@Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-11-13T09:21:05Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2379

Pearl1594 · 2020-11-13T09:22:17Z

@blueorangutan package

blueorangutan · 2020-11-13T09:23:35Z

@Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-11-13T09:59:09Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2380

DaanHoogland · 2020-11-13T10:03:44Z

 }

- if (userId != null && (accountId == null && domainId == null)) {
+ if (userId != null && (accountId == null || domainId == null)) {


why should they both be specified? user + either implies the other

discussed off-line with @Pearl1594 . no functional need but bugs may arise when not implemented this way

Pearl1594 · 2020-11-13T10:15:48Z

@blueorangutan test

blueorangutan · 2020-11-13T10:16:30Z

@Pearl1594 a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2020-11-13T19:52:35Z

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

Test	Result	Time (s)	Test File
test_04_rvpc_privategw_static_routes	`Failure`	239.45	test_privategw_acl.py

harikrishna-patnala

LGTM, test failure is not related to these changes.

DaanHoogland · 2020-11-16T08:02:47Z

travis has more errors than expected, retrying

DaanHoogland · 2020-11-16T08:05:25Z

this and #4276 look like they are related!?

sureshanaparti · 2020-11-16T08:58:21Z


- if (userId != null && (accountId == null && domainId == null)) {
+ if (userId != null && (accountId == null || domainId == null)) {
 throw new InvalidParameterValueException("Account ID and Domain ID must be specified with userID");


@Pearl1594 update this msg to reflect the change in cond.

It already did, as far as I can tell, neither accountId nor domainId may be null so both must be specified.

am i missing anything here @DaanHoogland , the cond. is OR and the msg says both (account id and domain id).

throw an exception when either is null, would mean both may not be null. The message was already the (not yet implemented) new condition.

hmmm , correct @DaanHoogland

DaanHoogland · 2020-11-16T11:45:53Z

no test effort shows in the comments or description @Pearl1594 . Can you add or chase some?
if you feel this is covered by smoke tests, please mention those here.
travis is lacking I think we can ignore it for this one.

Pearl1594 · 2020-11-16T11:48:52Z

no test effort shows in the comments or description @Pearl1594 . Can you add or chase some?
if you feel this is covered by smoke tests, please mention those here.
travis is lacking I think we can ignore it for this one.

@DaanHoogland Tests performed were listed in the following PR: #4316
I've just raised this PR as it was reverted while merging some code.

DaanHoogland · 2020-11-16T12:33:48Z

So are the same before: and after: entries applicable here @Pearl1594 ?

Pearl1594 · 2020-11-16T12:36:00Z

So are the same before: and after: entries applicable here @Pearl1594 ?

yes @DaanHoogland

weizhouapache · 2020-11-16T14:07:25Z

what about 4.14 ?

DaanHoogland · 2020-11-16T14:30:12Z

@weizhouapache this fixes a regression caused in a 4.15 commit on master. the original was also targeted on 4.15. not sure if it is applicable to 4.14. If it is it must be back-ported.

Pearl1594 · 2020-11-17T04:38:19Z

@DaanHoogland @weizhouapache this fix was specific to a feature introduced in 4.15.

weizhouapache · 2020-11-17T08:11:32Z

@DaanHoogland @Pearl1594 thanks, clear
it is a regression of #4307

Pearl1594 added the type:regression label Nov 13, 2020

Pearl1594 added this to the 4.15.0.0 milestone Nov 13, 2020

Pearl1594 requested review from DaanHoogland and harikrishna-patnala November 13, 2020 08:46

Pearl1594 changed the title ~~Regression fix~~Fix: Listing projects comprising of only users when listAll=true is passedNov 13, 2020

DaanHoogland added the type:bug label Nov 13, 2020

DaanHoogland approved these changes Nov 13, 2020
View reviewed changes

DaanHoogland changed the title ~~Fix: Listing projects comprising of only users when listAll=true is passed~~Fix: Listing projects comprising of only the user's on listAll=trueNov 13, 2020

Regression fix
ba11bdf

Pearl1594 force-pushed the fix-regression branch from c86264d to ba11bdfCompare November 13, 2020 09:21

DaanHoogland reviewed Nov 13, 2020
View reviewed changes

harikrishna-patnala approved these changes Nov 16, 2020
View reviewed changes

DaanHoogland closed this Nov 16, 2020

DaanHoogland reopened this Nov 16, 2020

sureshanaparti reviewed Nov 16, 2020
View reviewed changes

sureshanaparti approved these changes Nov 16, 2020
View reviewed changes

DaanHoogland added status:ready-for-merge status:needs-testing and removed status:ready-for-merge labels Nov 16, 2020

DaanHoogland removed the status:needs-testing label Nov 16, 2020

DaanHoogland merged commit 7f408ec into apache:masterNov 16, 2020

DaanHoogland deleted the fix-regression branch November 16, 2020 12:37

DaanHoogland mentioned this pull request Nov 17, 2020
Update listVirtualMachines API documentation with 'projectid=-1' to list all VMs in projects. #4276
Merged
5 tasks

Uh oh!

Conversation

Pearl1594 commented Nov 13, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Uh oh!

Pearl1594 commented Nov 13, 2020

Uh oh!

blueorangutan commented Nov 13, 2020

Uh oh!

blueorangutan commented Nov 13, 2020

Uh oh!

Pearl1594 commented Nov 13, 2020

Uh oh!

blueorangutan commented Nov 13, 2020

Uh oh!

blueorangutan commented Nov 13, 2020

Uh oh!

DaanHooglandNov 13, 2020

Choose a reason for hiding this comment

Uh oh!

DaanHooglandNov 13, 2020

Choose a reason for hiding this comment

Uh oh!

Pearl1594 commented Nov 13, 2020

Uh oh!

blueorangutan commented Nov 13, 2020

Uh oh!

blueorangutan commented Nov 13, 2020

Uh oh!

harikrishna-patnala left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Nov 16, 2020

Uh oh!

DaanHoogland commented Nov 16, 2020

Uh oh!

sureshanapartiNov 16, 2020

Choose a reason for hiding this comment

Uh oh!

DaanHooglandNov 16, 2020

Choose a reason for hiding this comment

Uh oh!

sureshanapartiNov 16, 2020

Choose a reason for hiding this comment

Uh oh!

DaanHooglandNov 16, 2020

Choose a reason for hiding this comment

Uh oh!

sureshanapartiNov 16, 2020

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Nov 16, 2020

Uh oh!

Pearl1594 commented Nov 16, 2020

Uh oh!

DaanHoogland commented Nov 16, 2020

Uh oh!

Pearl1594 commented Nov 16, 2020

Uh oh!

weizhouapache commented Nov 16, 2020

Uh oh!

DaanHoogland commented Nov 16, 2020

Uh oh!

Pearl1594 commented Nov 17, 2020

Uh oh!

weizhouapache commented Nov 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Pearl1594 commented Nov 13, 2020•
edited
Loading