-
Notifications
You must be signed in to change notification settings - Fork 1.2k
migrate Vmware to KVM ui issues #10413
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
migrate Vmware to KVM ui issues #10413
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.19 #10413 +/- ##
============================================
- Coverage 15.15% 15.15% -0.01%
+ Complexity 11318 11316 -2
============================================
Files 5413 5413
Lines 474670 474700 +30
Branches 57890 57893 +3
============================================
- Hits 71953 71939 -14
- Misses 394668 394716 +48
+ Partials 8049 8045 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@harikrishna-patnala a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress. |
|
UI build: ✔️ |
harikrishna-patnala
left a comment
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.
code LGTM.
I see the method is returning boolean but not used. Probably its good to change the method name listZoneVmwareDcHosts -> loadZoneVmwareDcHosts
kiranchavala
left a comment
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.
Before the fix, there are lot of notifications, when the vcenter password is entered
After fix, I have noticed that the message "No vmware hosts were in the selected Datacenter" when no details are given and also when details are provided
And on submitting List Vmware Instances , it list all the vm present in the datacenter
|
@kiranchavala, I made some changes to address your comments. Can you have another look? |
|
@DaanHoogland a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress. |
|
UI build: ✔️ |
|
@blueorangutan package |
|
@kiranchavala 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12500 |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12506 |
kiranchavala
left a comment
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.
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.
Could you please change the text from
No EXSi hosts were found in the selected Datacenter. Are the credentials complete?
to
No EXSi hosts were found in the selected Datacenter. Are the entered credentials correct?
Also if incorrect credentials are entered can cloudstack thrown a error notification ?
|
@sureshanaparti , is this ok by you now? |
sureshanaparti
left a comment
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.
clgtm
@DaanHoogland @kiranchavala on entry key in password field with incorrect credentials, throws error notification ? |
@sureshanaparti @kiranchavala , I experimented a bit, but it is hard to restrict the number of notifications thrown to just one, and the essence is also displayed in the label. So I am leaving that part for now. We can improve in the future. |
nvazquez
left a comment
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.
Code LGTM





Description
This PR...
Fixes: #10412
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?
in local UI pointing to a lab env