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/10/12 08:13:46 UTC
[GitHub] [cloudstack] ravening opened a new pull request #4396: Show network name in exception message
ravening opened a new pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396
## Description
<!--- Describe your changes in detail -->
Display network name in exception message instead of network object
<!-- 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: # -->
## 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)
## 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 -->
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#issuecomment-717930771
@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] weizhouapache commented on pull request #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#issuecomment-781980326
```
// * verify that there are no duplicates
if (hostNames.contains(hostName)) {
throw new InvalidParameterValueException("The vm with hostName " + hostName + " already exists in the network domain: " + ntwkDomain.getKey() + "; network="
- + (_networkModel.getNetwork(ntwkId) != null) ? _networkModel.getNetwork(ntwkId).getName()) : "<unknown>";
+ + (_networkModel.getNetwork(ntwkId) != null) ? _networkModel.getNetwork(ntwkId).getName() : "<unknown>");
}
}
}
```
----------------------------------------------------------------
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] ravening commented on a change in pull request #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#discussion_r503812081
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -3821,7 +3821,7 @@ private void checkIfHostNameUniqueInNtwkDomain(String hostName, List<? extends N
// * verify that there are no duplicates
if (hostNames.contains(hostName)) {
throw new InvalidParameterValueException("The vm with hostName " + hostName + " already exists in the network domain: " + ntwkDomain.getKey() + "; network="
- + _networkModel.getNetwork(ntwkId));
+ + _networkModel.getNetwork(ntwkId).getName());
Review comment:
> @ravening How about just adding the network name to the NetworkVO toString(), so this remains the same, and we get to see the network's name along with other details like networkId, traffic type, etc?
@Pearl1594 should we all those detail in exception message? It makes sense to add entire networkVO object tostring in log message
----------------------------------------------------------------
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] rhtyd commented on pull request #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#issuecomment-717884012
@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 a change in pull request #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#discussion_r503810344
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -3821,7 +3821,7 @@ private void checkIfHostNameUniqueInNtwkDomain(String hostName, List<? extends N
// * verify that there are no duplicates
if (hostNames.contains(hostName)) {
throw new InvalidParameterValueException("The vm with hostName " + hostName + " already exists in the network domain: " + ntwkDomain.getKey() + "; network="
- + _networkModel.getNetwork(ntwkId));
+ + _networkModel.getNetwork(ntwkId).getName());
Review comment:
@ravening How about just adding the network name to the NetworkVO toString(), so this remains the same, and we get to see the network's name along with other details like networkId, traffic type, etc?
----------------------------------------------------------------
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] ravening commented on a change in pull request #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#discussion_r503886888
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -3821,7 +3821,7 @@ private void checkIfHostNameUniqueInNtwkDomain(String hostName, List<? extends N
// * verify that there are no duplicates
if (hostNames.contains(hostName)) {
throw new InvalidParameterValueException("The vm with hostName " + hostName + " already exists in the network domain: " + ntwkDomain.getKey() + "; network="
- + _networkModel.getNetwork(ntwkId));
+ + _networkModel.getNetwork(ntwkId).getName());
Review comment:
> > > @ravening How about just adding the network name to the NetworkVO toString(), so this remains the same, and we get to see the network's name along with other details like networkId, traffic type, etc?
>
> >
>
> > @Pearl1594 should we all those detail in exception message? It makes sense to add entire networkVO object tostring in log message
>
>
>
> It actually depends, we may not want to show the network Id (i.e, DB id) to users, so maybe replace id with name in the toString()
That will involve extra code changes which I want to avoid
----------------------------------------------------------------
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 #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#issuecomment-717909117
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2296
----------------------------------------------------------------
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 #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#discussion_r503831503
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -3821,7 +3821,7 @@ private void checkIfHostNameUniqueInNtwkDomain(String hostName, List<? extends N
// * verify that there are no duplicates
if (hostNames.contains(hostName)) {
throw new InvalidParameterValueException("The vm with hostName " + hostName + " already exists in the network domain: " + ntwkDomain.getKey() + "; network="
- + _networkModel.getNetwork(ntwkId));
+ + _networkModel.getNetwork(ntwkId).getName());
Review comment:
> @DaanHoogland we are iterating over the list of networks which got populated earlier and it contains only valid existing networks. so networkmodel shouldnt return null
Maybe so @ravening, i trust your judgement on that. But it didn't happen in this method, so there is no telling what future generations will make of 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#discussion_r575140420
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -3821,7 +3821,7 @@ private void checkIfHostNameUniqueInNtwkDomain(String hostName, List<? extends N
// * verify that there are no duplicates
if (hostNames.contains(hostName)) {
throw new InvalidParameterValueException("The vm with hostName " + hostName + " already exists in the network domain: " + ntwkDomain.getKey() + "; network="
- + _networkModel.getNetwork(ntwkId));
+ + _networkModel.getNetwork(ntwkId).getName());
Review comment:
```suggestion
+ (_networkModel.getNetwork(ntwkId) != null) _networkModel.getNetwork(ntwkId).getName()) : "<unknown>";
```
----------------------------------------------------------------
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] rhtyd commented on pull request #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#issuecomment-717845826
@ravening can you fix 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#discussion_r575140420
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -3821,7 +3821,7 @@ private void checkIfHostNameUniqueInNtwkDomain(String hostName, List<? extends N
// * verify that there are no duplicates
if (hostNames.contains(hostName)) {
throw new InvalidParameterValueException("The vm with hostName " + hostName + " already exists in the network domain: " + ntwkDomain.getKey() + "; network="
- + _networkModel.getNetwork(ntwkId));
+ + _networkModel.getNetwork(ntwkId).getName());
Review comment:
```suggestion
+ (_networkModel.getNetwork(ntwkId) != null) ? _networkModel.getNetwork(ntwkId).getName()) : "<unknown>";
```
----------------------------------------------------------------
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] rhtyd commented on a change in pull request #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
rhtyd commented on a change in pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#discussion_r504554071
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -3821,7 +3821,7 @@ private void checkIfHostNameUniqueInNtwkDomain(String hostName, List<? extends N
// * verify that there are no duplicates
if (hostNames.contains(hostName)) {
throw new InvalidParameterValueException("The vm with hostName " + hostName + " already exists in the network domain: " + ntwkDomain.getKey() + "; network="
- + _networkModel.getNetwork(ntwkId));
+ + _networkModel.getNetwork(ntwkId).getName());
Review comment:
How about fixing/extending the toString of the returned network object (the VO?)
----------------------------------------------------------------
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 #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#issuecomment-717931326
@DaanHoogland 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] ravening commented on a change in pull request #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#discussion_r506442381
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -3821,7 +3821,7 @@ private void checkIfHostNameUniqueInNtwkDomain(String hostName, List<? extends N
// * verify that there are no duplicates
if (hostNames.contains(hostName)) {
throw new InvalidParameterValueException("The vm with hostName " + hostName + " already exists in the network domain: " + ntwkDomain.getKey() + "; network="
- + _networkModel.getNetwork(ntwkId));
+ + _networkModel.getNetwork(ntwkId).getName());
Review comment:
> @ravening while it does make sense to show the network name, we do not ensure uniqueness in network names in CloudStack - so would it lead to ambiguity just showing the name?
@Pearl1594 this exception is thrown when we are trying to create second VM with same in same network. So I would assume, user will know in which network he is creating the VM. Also even if the network name is not unique, it's better than displaying VO object right which won't make sense anyway
----------------------------------------------------------------
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 #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#discussion_r503271152
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -3821,7 +3821,7 @@ private void checkIfHostNameUniqueInNtwkDomain(String hostName, List<? extends N
// * verify that there are no duplicates
if (hostNames.contains(hostName)) {
throw new InvalidParameterValueException("The vm with hostName " + hostName + " already exists in the network domain: " + ntwkDomain.getKey() + "; network="
- + _networkModel.getNetwork(ntwkId));
+ + _networkModel.getNetwork(ntwkId).getName());
Review comment:
this might yield an NPE hiding the original problem, no?
----------------------------------------------------------------
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] rhtyd merged pull request #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
rhtyd merged pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396
----------------------------------------------------------------
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 #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#discussion_r503852917
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -3821,7 +3821,7 @@ private void checkIfHostNameUniqueInNtwkDomain(String hostName, List<? extends N
// * verify that there are no duplicates
if (hostNames.contains(hostName)) {
throw new InvalidParameterValueException("The vm with hostName " + hostName + " already exists in the network domain: " + ntwkDomain.getKey() + "; network="
- + _networkModel.getNetwork(ntwkId));
+ + _networkModel.getNetwork(ntwkId).getName());
Review comment:
> > @ravening How about just adding the network name to the NetworkVO toString(), so this remains the same, and we get to see the network's name along with other details like networkId, traffic type, etc?
>
> @Pearl1594 should we all those detail in exception message? It makes sense to add entire networkVO object tostring in log message
It actually depends, we may not want to show the network Id (i.e, DB id) to users, so maybe replace id with name in the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#discussion_r503800086
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -3821,7 +3821,7 @@ private void checkIfHostNameUniqueInNtwkDomain(String hostName, List<? extends N
// * verify that there are no duplicates
if (hostNames.contains(hostName)) {
throw new InvalidParameterValueException("The vm with hostName " + hostName + " already exists in the network domain: " + ntwkDomain.getKey() + "; network="
- + _networkModel.getNetwork(ntwkId));
+ + _networkModel.getNetwork(ntwkId).getName());
Review comment:
@DaanHoogland we are iterating over the list of networks which got populated earlier and it contains only valid existing networks. so networkmodel shouldnt return null
----------------------------------------------------------------
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 #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#discussion_r503796696
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -3821,7 +3821,7 @@ private void checkIfHostNameUniqueInNtwkDomain(String hostName, List<? extends N
// * verify that there are no duplicates
if (hostNames.contains(hostName)) {
throw new InvalidParameterValueException("The vm with hostName " + hostName + " already exists in the network domain: " + ntwkDomain.getKey() + "; network="
- + _networkModel.getNetwork(ntwkId));
+ + _networkModel.getNetwork(ntwkId).getName());
Review comment:
I am talking about the model returning a network in the first place. the namer being null will probably just print `"<null>"`, @ravening @GabrielBrascher
----------------------------------------------------------------
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] ravening commented on pull request #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
ravening commented on pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#issuecomment-709944933
> Thanks for the PR @ravening.
>
> I am **+1** on adding the network's name on exception. I did not add a _looks good_ for this PR in respect to Daan's comment.
@GabrielBrascher Daan was mentioning about networkmodel retuning a null rather than network name being null. So i think this should be fine and no need for extra if check
----------------------------------------------------------------
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] ravening commented on a change in pull request #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#discussion_r503298089
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -3821,7 +3821,7 @@ private void checkIfHostNameUniqueInNtwkDomain(String hostName, List<? extends N
// * verify that there are no duplicates
if (hostNames.contains(hostName)) {
throw new InvalidParameterValueException("The vm with hostName " + hostName + " already exists in the network domain: " + ntwkDomain.getKey() + "; network="
- + _networkModel.getNetwork(ntwkId));
+ + _networkModel.getNetwork(ntwkId).getName());
Review comment:
@DaanHoogland @GabrielBrascher most of the time the network name will be nonnull but there is no harm adding an extra if check. I will modify the 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] ravening commented on pull request #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
ravening commented on pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#issuecomment-717883661
> @ravening can you fix the conflict?
@rhtyd done
----------------------------------------------------------------
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 #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#issuecomment-717884602
@rhtyd 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] Pearl1594 commented on a change in pull request #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#discussion_r506435817
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -3821,7 +3821,7 @@ private void checkIfHostNameUniqueInNtwkDomain(String hostName, List<? extends N
// * verify that there are no duplicates
if (hostNames.contains(hostName)) {
throw new InvalidParameterValueException("The vm with hostName " + hostName + " already exists in the network domain: " + ntwkDomain.getKey() + "; network="
- + _networkModel.getNetwork(ntwkId));
+ + _networkModel.getNetwork(ntwkId).getName());
Review comment:
@ravening while it does make sense to show the network name, we do not ensure uniqueness in network names in CloudStack - so would it lead to ambiguity just showing the name?
----------------------------------------------------------------
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] GabrielBrascher commented on a change in pull request #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#discussion_r503293633
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -3821,7 +3821,7 @@ private void checkIfHostNameUniqueInNtwkDomain(String hostName, List<? extends N
// * verify that there are no duplicates
if (hostNames.contains(hostName)) {
throw new InvalidParameterValueException("The vm with hostName " + hostName + " already exists in the network domain: " + ntwkDomain.getKey() + "; network="
- + _networkModel.getNetwork(ntwkId));
+ + _networkModel.getNetwork(ntwkId).getName());
Review comment:
I see no problem in adding an *if* checking for a non-null name. However, a network can only be created with a name.
If the network is a non-null Object then I don't see how the name would be null. Unless manual queries have been executed on DB (messing with networks) or there are some bugs on the network removal/update process.
----------------------------------------------------------------
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 #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#issuecomment-718252210
<b>Trillian test result (tid-3097)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31757 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4396-t3097-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` | 869.32 | 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] weizhouapache commented on pull request #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#issuecomment-781972718
@rhtyd @ravening
build failed due to a missing ")" at the end of line.
----------------------------------------------------------------
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 #4396: Show network name in exception message
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #4396:
URL: https://github.com/apache/cloudstack/pull/4396#issuecomment-781980974
good it has been fixed by @rhtyd
----------------------------------------------------------------
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