You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2020/11/13 08:46:28 UTC
[GitHub] [cloudstack] Pearl1594 opened a new pull request #4469: Regression fix
Pearl1594 opened a new pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469
## Description
A few changes made to `server/src/main/java/com/cloud/api/query/QueryManagerImpl.java` were reverted by commit: `fc05d3168f14584431a1a6b3c1f88cf04999cfaa`
## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [X] 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)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#issuecomment-727928274
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#discussion_r524019362
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/project/CreateProjectCmd.java
##########
@@ -116,7 +116,7 @@ public long getEntityOwnerId() {
throw new InvalidParameterValueException("Account name and domain id must be specified together");
}
- 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");
Review comment:
It already did, as far as I can tell, neither accountId nor domainId may be null so both must be specified.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 commented on pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#issuecomment-727929589
> 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: https://github.com/apache/cloudstack/pull/4316
I've just raised this PR as it was reverted while merging some code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#discussion_r522848665
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/project/CreateProjectCmd.java
##########
@@ -116,7 +116,7 @@ public long getEntityOwnerId() {
throw new InvalidParameterValueException("Account name and domain id must be specified together");
}
- if (userId != null && (accountId == null && domainId == null)) {
+ if (userId != null && (accountId == null || domainId == null)) {
Review comment:
ignore, bad reading on my part
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#issuecomment-727950550
So are the same `before:` and `after:` entries applicable here @Pearl1594 ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 commented on a change in pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#discussion_r522854520
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/project/CreateProjectCmd.java
##########
@@ -116,7 +116,7 @@ public long getEntityOwnerId() {
throw new InvalidParameterValueException("Account name and domain id must be specified together");
}
- if (userId != null && (accountId == null && domainId == null)) {
+ if (userId != null && (accountId == null || domainId == null)) {
Review comment:
@DaanHoogland this was the requirement in the FR doc
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#issuecomment-728762640
@DaanHoogland @Pearl1594 thanks, clear
it is a regression of #4307
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#issuecomment-726999641
<b>Trillian test result (tid-3167)</b>
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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#issuecomment-726651485
@Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#issuecomment-726649028
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2379
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#discussion_r522848532
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/project/CreateProjectCmd.java
##########
@@ -116,7 +116,7 @@ public long getEntityOwnerId() {
throw new InvalidParameterValueException("Account name and domain id must be specified together");
}
- if (userId != null && (accountId == null && domainId == null)) {
+ if (userId != null && (accountId == null || domainId == null)) {
Review comment:
why should they both be specified? user + either implies the other
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#issuecomment-726678226
@Pearl1594 a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 commented on pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#issuecomment-726650156
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#discussion_r523987964
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/project/CreateProjectCmd.java
##########
@@ -116,7 +116,7 @@ public long getEntityOwnerId() {
throw new InvalidParameterValueException("Account name and domain id must be specified together");
}
- 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");
Review comment:
@Pearl1594 update this msg to reflect the change in cond.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#issuecomment-727808450
travis has more errors than expected, retrying
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#discussion_r522866128
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/project/CreateProjectCmd.java
##########
@@ -116,7 +116,7 @@ public long getEntityOwnerId() {
throw new InvalidParameterValueException("Account name and domain id must be specified together");
}
- if (userId != null && (accountId == null && domainId == null)) {
+ if (userId != null && (accountId == null || domainId == null)) {
Review comment:
discussed off-line with @Pearl1594 . no functional need but bugs may arise when not implemented this way
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#discussion_r524025843
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/project/CreateProjectCmd.java
##########
@@ -116,7 +116,7 @@ public long getEntityOwnerId() {
throw new InvalidParameterValueException("Account name and domain id must be specified together");
}
- 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");
Review comment:
am i missing anything here @DaanHoogland , the cond. is OR and the msg says both (account id and domain id).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#issuecomment-726668919
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2380
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#discussion_r524101367
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/project/CreateProjectCmd.java
##########
@@ -116,7 +116,7 @@ public long getEntityOwnerId() {
throw new InvalidParameterValueException("Account name and domain id must be specified together");
}
- 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");
Review comment:
hmmm , correct @DaanHoogland
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 commented on pull request #4469: Regression fix
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#issuecomment-726622529
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 commented on pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#issuecomment-726677824
@blueorangutan test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 commented on a change in pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#discussion_r522854520
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/project/CreateProjectCmd.java
##########
@@ -116,7 +116,7 @@ public long getEntityOwnerId() {
throw new InvalidParameterValueException("Account name and domain id must be specified together");
}
- if (userId != null && (accountId == null && domainId == null)) {
+ if (userId != null && (accountId == null || domainId == null)) {
Review comment:
@DaanHoogland this was the requirement in the FR doc
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 commented on pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#issuecomment-728680948
@DaanHoogland @weizhouapache this fix was specific to a feature introduced in 4.15.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4469: Regression fix
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#issuecomment-726623355
@Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland merged pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
DaanHoogland merged pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland closed pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
DaanHoogland closed pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#issuecomment-728098165
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#issuecomment-728076883
what about 4.14 ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#discussion_r524074866
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/project/CreateProjectCmd.java
##########
@@ -116,7 +116,7 @@ public long getEntityOwnerId() {
throw new InvalidParameterValueException("Account name and domain id must be specified together");
}
- 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");
Review comment:
throw an exception when either is null, would mean both may not be null. The message was already the (not yet implemented) new condition.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#issuecomment-727809794
this and #4276 look like they are related!?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 commented on pull request #4469: Fix: Listing projects comprising of only the user's on listAll=true
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #4469:
URL: https://github.com/apache/cloudstack/pull/4469#issuecomment-727951601
> So are the same `before:` and `after:` entries applicable here @Pearl1594 ?
yes @DaanHoogland
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org