You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "isupersky (via GitHub)" <gi...@apache.org> on 2023/11/09 10:43:25 UTC
[PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
isupersky opened a new pull request, #8207:
URL: https://github.com/apache/cloudstack/pull/8207
### Description
- Adding a default method setDomainPath in ControlledEntityResponse to avoid creating a variable and setter definitions in all implementations of ControlledEntityResponse class.
- and populated the same in createNetworkResponse method of ApiResponseHelper
-
<!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
<!--- ********************************************************************************* -->
<!--- NOTE: AUTOMATATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ********************************************************************************* -->
### 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)
- [ ] build/CI
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [ ] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
Created a guest network and called the `list networks filter=name,type,domainpath` before and after the code change
#### How did you try to break this feature and the system with this change?
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1807541116
@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. 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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1812111364
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "isupersky (via GitHub)" <gi...@apache.org>.
isupersky commented on code in PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#discussion_r1388507028
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -2598,6 +2598,9 @@ public NetworkResponse createNetworkResponse(ResponseView view, Network network)
if (domain != null) {
response.setDomainId(domain.getUuid());
response.setDomainName(domain.getName());
+ StringBuilder domainPath = new StringBuilder("ROOT");
Review Comment:
@DaanHoogland Please correct me if I'm wrong
what I understand is -
- "ROOT" should not be prepended in any domainPath added in this PR.
- and if somewhere this has been done previously also then it should be replaced with "/"
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#discussion_r1389551742
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -2598,6 +2598,9 @@ public NetworkResponse createNetworkResponse(ResponseView view, Network network)
if (domain != null) {
response.setDomainId(domain.getUuid());
response.setDomainName(domain.getName());
+ StringBuilder domainPath = new StringBuilder("ROOT");
Review Comment:
You can have a look at the uses of `ConfigKey<>` , you'd be using something like `defaultRootDomainName.value()` and the operator can than change that. But let's not do that. It is fine like so.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1812555101
@DaanHoogland a [SL] 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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1807592114
Packaging result [SF]: :heavy_multiplication_x: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 7727
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1812538592
Packaging result [SF]: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 7760
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1809687722
@rohityadavcloud a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. 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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1812126955
@isupersky can you resolve the conflict?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse, Overridden in NetworkRespose class and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-3645516903
@isupersky , closing this one as it has conflicts and is old. please update and reopen if you think it is still relevant.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse, Overridden in NetworkRespose class and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "vishesh92 (via GitHub)" <gi...@apache.org>.
vishesh92 commented on code in PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#discussion_r1399118134
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -5109,4 +5108,15 @@ public IpQuarantineResponse createQuarantinedIpsResponse(PublicIpQuarantine quar
return quarantinedIpsResponse;
}
+
+ private String getDomainPath(String path){
+ StringBuilder domainPath = new StringBuilder();
+ if(ObjectUtils.isEmpty(path) || path.equals("/")){
Review Comment:
IMO. Since we know that path is a string, better to use `org.apache.commons.lang3.StringUtils.isEmpty()`..
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#discussion_r1395319644
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -2585,10 +2585,7 @@ public NetworkResponse createNetworkResponse(ResponseView view, Network network)
Domain domain = ApiDBUtils.findDomainById(domainNetworkDetails.first());
if (domain != null) {
response.setDomainId(domain.getUuid());
-
- StringBuilder domainPath = new StringBuilder("ROOT");
- (domainPath.append(domain.getPath())).deleteCharAt(domainPath.length() - 1);
- response.setDomainPath(domainPath.toString());
+ response.setDomainPath(getDomainPath(domain.getPath()));
Review Comment:
is there any functional reason to prepend "ROOT"? It is already equivilent with "/", functionally and the current code returns an empty string.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse, Overridden in NetworkRespose class and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1826285176
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse, Overridden in NetworkRespose class and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1827146513
@shwstppr @vishesh92 Are you alright with the code like this?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "isupersky (via GitHub)" <gi...@apache.org>.
isupersky commented on code in PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#discussion_r1391049418
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -2583,9 +2583,13 @@ public NetworkResponse createNetworkResponse(ResponseView view, Network network)
Domain domain = ApiDBUtils.findDomainById(domainNetworkDetails.first());
if (domain != null) {
response.setDomainId(domain.getUuid());
-
- StringBuilder domainPath = new StringBuilder("ROOT");
- (domainPath.append(domain.getPath())).deleteCharAt(domainPath.length() - 1);
+ StringBuilder domainPath = new StringBuilder();
+ if(ObjectUtils.isEmpty(domain.getPath()) || domain.getPath().equals("/")){
Review Comment:
@DaanHoogland there are two variables here, domain.getPath() and domainPath.
domainPath is initially empty `StringBuilder domainPath = new StringBuilder();` , hence it will only be initialised with single "/".
Thanks for the second suggestion, I'll move the code repetition into a method.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse, Overridden in NetworkRespose class and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1840121914
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse, Overridden in NetworkRespose class and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1840231006
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7918
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#discussion_r1389033736
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -2598,6 +2598,9 @@ public NetworkResponse createNetworkResponse(ResponseView view, Network network)
if (domain != null) {
response.setDomainId(domain.getUuid());
response.setDomainName(domain.getName());
+ StringBuilder domainPath = new StringBuilder("ROOT");
Review Comment:
no, that is not what I meant
if the root domain is the it we return "/" or instead "ROOT", or even "".
in all other cases we return something like "/domain/subdomain/..."
I would not bother about other places withing the scope of this PR (unless you feel like it)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1812451042
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1812552069
@blueorangutan test keepEnv
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse, Overridden in NetworkRespose class and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland closed pull request #8207: Adding a default setter for domainPath in ControlledEntityResponse, Overridden in NetworkRespose class and populated the same in createNetworkResponse method of ApiResponseHelper #8086
URL: https://github.com/apache/cloudstack/pull/8207
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse, Overridden in NetworkRespose class and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-2943570785
hi @isupersky please fix the conflicts in the branch.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse, Overridden in NetworkRespose class and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1826670199
<b>[SF] Trillian test result (tid-8392)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 45011 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8207-t8392-kvm-centos7.zip
Smoke tests completed. 117 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_08_migrate_vm | `Error` | 0.05 | test_vm_life_cycle.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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse, Overridden in NetworkRespose class and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#discussion_r1413512207
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -5159,4 +5158,15 @@ public BucketResponse createBucketResponse(Bucket bucket) {
populateAccount(bucketResponse, bucket.getAccountId());
return bucketResponse;
}
+
+ private String getDomainPath(String path){
+ StringBuilder domainPath = new StringBuilder();
+ if (StringUtils.isEmpty(path) || path.equals("/")){
+ domainPath.append("/");
+ }
+ else{
Review Comment:
@isupersky , @vishesh92 meant also to change
```suggestion
else {
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse, Overridden in NetworkRespose class and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1840133884
@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. 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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "isupersky (via GitHub)" <gi...@apache.org>.
isupersky commented on code in PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#discussion_r1389510015
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -2598,6 +2598,9 @@ public NetworkResponse createNetworkResponse(ResponseView view, Network network)
if (domain != null) {
response.setDomainId(domain.getUuid());
response.setDomainName(domain.getName());
+ StringBuilder domainPath = new StringBuilder("ROOT");
Review Comment:
made the changes.
Not sure, what you mean by configurable here. is it that rather than using a string "/" multiple times, i should extract it in single variable?
something like -> `private static final String defaultDomainPath = "/";`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1809772881
Packaging result [SF]: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 7741
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse, Overridden in NetworkRespose class and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-2310501088
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1813708897
<b>[SF] Trillian test result (tid-8328)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 46033 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8207-t8328-kvm-centos7.zip
Smoke tests completed. 118 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse, Overridden in NetworkRespose class and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "vishesh92 (via GitHub)" <gi...@apache.org>.
vishesh92 commented on code in PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#discussion_r1413513103
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -5159,4 +5158,15 @@ public BucketResponse createBucketResponse(Bucket bucket) {
populateAccount(bucketResponse, bucket.getAccountId());
return bucketResponse;
}
+
+ private String getDomainPath(String path){
+ StringBuilder domainPath = new StringBuilder();
+ if (StringUtils.isEmpty(path) || path.equals("/")){
+ domainPath.append("/");
+ }
+ else{
+ (domainPath.append(path)).deleteCharAt(domainPath.length() - 1);
Review Comment:
```suggestion
if (StringUtils.isEmpty(path) || path.equals("/")) {
domainPath.append("/");
} else {
(domainPath.append(path)).deleteCharAt(domainPath.length() - 1);
```
@isupersky please note the code style changes.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "isupersky (via GitHub)" <gi...@apache.org>.
isupersky commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1818343919
> Adding setDomainPath() method in ControlledEntityResponse makes sense but it is an interface and the method is not overridden in any of the implemented classes. Only NetworkResponse will have domainpath param which was having it even before this PR and the method there as well is not overridden.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "isupersky (via GitHub)" <gi...@apache.org>.
isupersky closed pull request #8207: Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086
URL: https://github.com/apache/cloudstack/pull/8207
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1818401833
> > Adding setDomainPath() method in ControlledEntityResponse makes sense but it is an interface and the method is not overridden in any of the implemented classes. Only NetworkResponse will have domainpath param which was having it even before this PR and the method there as well is not overridden.
>
> @shwstppr I added it at two new flows of createNetworkResponse in ApiResponseHelper. I'm not sure which other implementation class would require domainpath for ListNetwork API ?
In that case please update PR title/description that the change is limited to listNetworks API and not all implementations of ControlledEntityResponse class
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#discussion_r1390886990
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -2583,9 +2583,13 @@ public NetworkResponse createNetworkResponse(ResponseView view, Network network)
Domain domain = ApiDBUtils.findDomainById(domainNetworkDetails.first());
if (domain != null) {
response.setDomainId(domain.getUuid());
-
- StringBuilder domainPath = new StringBuilder("ROOT");
- (domainPath.append(domain.getPath())).deleteCharAt(domainPath.length() - 1);
+ StringBuilder domainPath = new StringBuilder();
+ if(ObjectUtils.isEmpty(domain.getPath()) || domain.getPath().equals("/")){
Review Comment:
not sure what you intent to do here @isupersky . it seems now that you append / to / resulting in //???
also you keep changing this in three locations. Would be good to have a method so we can change the code as needed in only one location.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse, Overridden in NetworkRespose class and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1826387648
@DaanHoogland a [SL] 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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse, Overridden in NetworkRespose class and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1836125755
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse, Overridden in NetworkRespose class and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "vishesh92 (via GitHub)" <gi...@apache.org>.
vishesh92 commented on code in PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#discussion_r1410522236
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -5109,4 +5108,15 @@ public IpQuarantineResponse createQuarantinedIpsResponse(PublicIpQuarantine quar
return quarantinedIpsResponse;
}
+
+ private String getDomainPath(String path){
+ StringBuilder domainPath = new StringBuilder();
+ if(StringUtils.isEmpty(path) || path.equals("/")){
+ domainPath.append("/");
+ }
+ else{
Review Comment:
```suggestion
if (StringUtils.isEmpty(path) || path.equals("/")) {
domainPath.append("/");
} else {
```
minor code style change.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse, Overridden in NetworkRespose class and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1826387326
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1818388761
@shwstppr you "requested changes" but it not entirely clear what changes. Do you require "ROOT" to be returned? As said in https://github.com/apache/cloudstack/pull/8207#discussion_r1395338161, I think this should only be so in the UI.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "isupersky (via GitHub)" <gi...@apache.org>.
isupersky commented on code in PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#discussion_r1389328526
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -2598,6 +2598,9 @@ public NetworkResponse createNetworkResponse(ResponseView view, Network network)
if (domain != null) {
response.setDomainId(domain.getUuid());
response.setDomainName(domain.getName());
+ StringBuilder domainPath = new StringBuilder("ROOT");
Review Comment:
@DaanHoogland I have updated the code. Please review
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#discussion_r1389390590
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -2598,6 +2598,9 @@ public NetworkResponse createNetworkResponse(ResponseView view, Network network)
if (domain != null) {
response.setDomainId(domain.getUuid());
response.setDomainName(domain.getName());
+ StringBuilder domainPath = new StringBuilder("ROOT");
Review Comment:
i like that, to creep the scope a bit, we could even make the "/" a configuration item ;)
but lgtm!!!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#discussion_r1395338161
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -2585,10 +2585,7 @@ public NetworkResponse createNetworkResponse(ResponseView view, Network network)
Domain domain = ApiDBUtils.findDomainById(domainNetworkDetails.first());
if (domain != null) {
response.setDomainId(domain.getUuid());
-
- StringBuilder domainPath = new StringBuilder("ROOT");
- (domainPath.append(domain.getPath())).deleteCharAt(domainPath.length() - 1);
- response.setDomainPath(domainPath.toString());
+ response.setDomainPath(getDomainPath(domain.getPath()));
Review Comment:
I think "ROOT" is a representational depiction of "/", and should be only shown in the UI. It makes no sense in the API and leads to less convenient processing. I would not want it in an API. Right now the response parameter `domainpath` is empty. I tested the difference:
```
(localcloud) 🐱 > list networks filter=domainpath
```
before:
```
{
"count": 1,
"network": [
{}
]
}
```
after:
```
{
"count": 1,
"network": [
{
"domainpath": "/"
}
]
}
```
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -2585,10 +2585,7 @@ public NetworkResponse createNetworkResponse(ResponseView view, Network network)
Domain domain = ApiDBUtils.findDomainById(domainNetworkDetails.first());
if (domain != null) {
response.setDomainId(domain.getUuid());
-
- StringBuilder domainPath = new StringBuilder("ROOT");
- (domainPath.append(domain.getPath())).deleteCharAt(domainPath.length() - 1);
- response.setDomainPath(domainPath.toString());
+ response.setDomainPath(getDomainPath(domain.getPath()));
Review Comment:
I think "ROOT" is a representational depiction of "/", and should be only shown in the UI. It makes no sense in the API and leads to less convenient processing. I would not want it in an API. Right now the response parameter `domainpath` is empty. I tested the difference:
```
(localcloud) 🐱 > list networks filter=domainpath
```
before:
```
{
"count": 1,
"network": [
{}
]
}
```
after:
```
{
"count": 1,
"network": [
{
"domainpath": "/"
}
]
}
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "isupersky (via GitHub)" <gi...@apache.org>.
isupersky commented on code in PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#discussion_r1389385297
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -2598,6 +2598,9 @@ public NetworkResponse createNetworkResponse(ResponseView view, Network network)
if (domain != null) {
response.setDomainId(domain.getUuid());
response.setDomainName(domain.getName());
+ StringBuilder domainPath = new StringBuilder("ROOT");
Review Comment:
yes, or else we can go with ->
`StringBuilder domainPath = new StringBuilder(); `
`if(ObjectUtils.isEmpty(domain.getPath())){`
` domainPath.append("/");`
` }`
`else{`
` (domainPath.append(domain.getPath())).deleteCharAt(domainPath.length() - 1);`
` }`
`response.setDomainPath(domainPath.toString());`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1803620913
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/8207?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#8207](https://app.codecov.io/gh/apache/cloudstack/pull/8207?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (73c5290) into [main](https://app.codecov.io/gh/apache/cloudstack/commit/29a223bf9b75bf62099c227b95d75cae65a1cf62?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (29a223b) will **decrease** coverage by `24.63%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## main #8207 +/- ##
============================================
- Coverage 29.12% 4.50% -24.63%
============================================
Files 5186 354 -4832
Lines 365754 27872 -337882
Branches 53489 4868 -48621
============================================
- Hits 106518 1255 -105263
+ Misses 244628 26479 -218149
+ Partials 14608 138 -14470
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/8207/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [simulator-marvin-tests](https://app.codecov.io/gh/apache/cloudstack/pull/8207/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/8207/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.50% <ø> (ø)` | |
| [unit-tests](https://app.codecov.io/gh/apache/cloudstack/pull/8207/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
[see 4832 files with indirect coverage changes](https://app.codecov.io/gh/apache/cloudstack/pull/8207/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in [Chrome](https://chrome.google.com/webstore/detail/codecov/gedikamndpbemklijjkncpnolildpbgo) or [Firefox](https://addons.mozilla.org/en-US/firefox/addon/codecov/) today!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#discussion_r1389367668
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -2598,6 +2598,9 @@ public NetworkResponse createNetworkResponse(ResponseView view, Network network)
if (domain != null) {
response.setDomainId(domain.getUuid());
response.setDomainName(domain.getName());
+ StringBuilder domainPath = new StringBuilder("ROOT");
Review Comment:
looks like you went for the empty string right?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on code in PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#discussion_r1395323127
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -2585,10 +2585,7 @@ public NetworkResponse createNetworkResponse(ResponseView view, Network network)
Domain domain = ApiDBUtils.findDomainById(domainNetworkDetails.first());
if (domain != null) {
response.setDomainId(domain.getUuid());
-
- StringBuilder domainPath = new StringBuilder("ROOT");
- (domainPath.append(domain.getPath())).deleteCharAt(domainPath.length() - 1);
- response.setDomainPath(domainPath.toString());
+ response.setDomainPath(getDomainPath(domain.getPath()));
Review Comment:
@DaanHoogland cc @isupersky I'm okay either. My only concern was that users/API consumers would start seeing a difference. Will that be an issue?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on code in PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#discussion_r1395174935
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -2585,10 +2585,7 @@ public NetworkResponse createNetworkResponse(ResponseView view, Network network)
Domain domain = ApiDBUtils.findDomainById(domainNetworkDetails.first());
if (domain != null) {
response.setDomainId(domain.getUuid());
-
- StringBuilder domainPath = new StringBuilder("ROOT");
- (domainPath.append(domain.getPath())).deleteCharAt(domainPath.length() - 1);
- response.setDomainPath(domainPath.toString());
+ response.setDomainPath(getDomainPath(domain.getPath()));
Review Comment:
Didn't test but will this cause a change in response with `ROOT` not getting prepended
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse, Overridden in NetworkRespose class and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1826285450
@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. 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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse, Overridden in NetworkRespose class and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1826295716
Packaging result [SF]: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 7828
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#discussion_r1387908002
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -2598,6 +2598,9 @@ public NetworkResponse createNetworkResponse(ResponseView view, Network network)
if (domain != null) {
response.setDomainId(domain.getUuid());
response.setDomainName(domain.getName());
+ StringBuilder domainPath = new StringBuilder("ROOT");
Review Comment:
@isupersky , I think we should not prepend "ROOT" in case there is any other path following (like "ROOT/mydomain").
We could replace if it is "/", I have no preference on that.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1807539655
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1809685668
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "isupersky (via GitHub)" <gi...@apache.org>.
isupersky commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1818394751
> Adding setDomainPath() method in ControlledEntityResponse makes sense but it is an interface and the method is not overridden in any of the implemented classes. Only NetworkResponse will have domainpath param which was having it even before this PR and the method there as well is not overridden.
@shwstppr I added it at two new flows of createNetworkResponse in ApiResponseHelper. I'm not sure which other implementation class would require domainpath for ListNetwork API ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse, Overridden in NetworkRespose class and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1838029439
@vishesh92 @shwstppr can you approve of this now?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#issuecomment-1812451459
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. 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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding a default setter for domainPath in ControlledEntityResponse, Overridden in NetworkRespose class and populated the same in createNetworkResponse method of ApiResponseHelper #8086 [cloudstack]
Posted by "isupersky (via GitHub)" <gi...@apache.org>.
isupersky commented on code in PR #8207:
URL: https://github.com/apache/cloudstack/pull/8207#discussion_r1414931630
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -5159,4 +5158,15 @@ public BucketResponse createBucketResponse(Bucket bucket) {
populateAccount(bucketResponse, bucket.getAccountId());
return bucketResponse;
}
+
+ private String getDomainPath(String path){
+ StringBuilder domainPath = new StringBuilder();
+ if (StringUtils.isEmpty(path) || path.equals("/")){
+ domainPath.append("/");
+ }
+ else{
Review Comment:
sorry, missed that. pushing the changes right away
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org