-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CLOUDSTACK-9730: [VMware] Unable to add a host with space in its name to existing VMware cluster #1891
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for code changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add unit test case(s) to validate the changes to validateCluster
method. I would recommend changing the visibility to default to make it easier to test.
@rhtyd Can you kick off VMware CI on this PR. |
@blueorangutan package |
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
@sureshanaparti since we're making changes of the return type can we also create/edit unit tests? |
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-510 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sureshanaparti can you please add the tests so I could kick integration and smoke tests. Thanks
@jburwell Thanks for the recommendation. Not changing the visibility for now. Couldn't find any tests for VmwareServerDiscoverer to update. Will try to add. |
@borisstoyanov the existing tests with space in the host name should work. |
@borisstoyanov Can you please kick off tests for this. |
@blueorangutan package |
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-590 |
@borisstoyanov unsupported parameters provided. Supported mgmt server os are: |
@blueorangutan test centos7 vmware-55u3 |
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + vmware-55u3) has been kicked to run smoke tests |
Trillian test result (tid-952)
|
The trillian test failures above are not related this PR changes. |
… to Cluster Skip update of encoded cluster url path in cluster_details table while adding host to existing cluster. Previously cluster url from API used to be inserted into DB after validation of inventory url. White spaces in url would be encoded as '+' during creation of URI object which are being inserted into database. Further references of the cluster's data center name would see '+' symbols instead of white space which is incorrect.
@blueorangutan package |
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1419 |
@blueorangutan package |
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1539 |
@blueorangutan package |
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1581 |
@blueorangutan test centos7 vmware-55u3 |
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + vmware-55u3) has been kicked to run smoke tests |
Trillian test result (tid-2036)
|
if (!URLDecoder.decode(url.getPath(), "UTF-8").equals(updatedInventoryPath)) { | ||
// If url from API doesn't specify DC then update url in database with DC associated with this zone. | ||
clusterDetails.put("url", url.getScheme() + "://" + url.getHost() + updatedInventoryPath); | ||
_clusterDetailsDao.persist(clusterId, clusterDetails); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sureshanaparti could not persisting the url in the cluster details cause regression?
Moving to 5.0.0 due to lack of activity |
I'm dropping this from a milestone and to be honest I would like to close it as:
|
@sureshanaparti please rebase and re-open if this is still relevant |
Issue: Unable to add a host with space in its name to existing VMware cluster
Root Cause: Previously cluster url from API used to be inserted into DB after validation of inventory url. White spaces in url would be encoded as '+' during creation of URI object which are being inserted into database. Further references of the cluster's data center name would see '+' symbols instead of white space which is incorrect.
Fix: Skip update of encoded cluster url path in cluster_details table while adding host to existing cluster.