Fix PR 7131 bugs and vulnerabilities - #7140
Conversation
Codecov Report
@@ Coverage Diff @@
## main #7140 +/- ##
============================================
+ Coverage 11.78% 12.63% +0.85%
- Complexity 7676 8613 +937
============================================
Files 2505 2711 +206
Lines 246339 255756 +9417
Branches 38427 39868 +1441
============================================
+ Hits 29039 32325 +3286
- Misses 213520 219324 +5804
- Partials 3780 4107 +327
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
DaanHoogland
left a comment
DaanHoogland
left a comment
There was a problem hiding this comment.
some suggestions but clgtm
| String secStorageVmHostName = ""; | ||
| String secStorageVmPublicIpAddress = ""; | ||
| String secStorageVmPrivateIpAddress = "N/A"; | ||
| Long secStorageVmPodIdToDeployIn = null; | ||
|
|
||
| if (secStorageVm != null) { | ||
| secStorageVmHostName = secStorageVm.getHostName(); | ||
| secStorageVmPublicIpAddress = secStorageVm.getPublicIpAddress(); | ||
| secStorageVmPrivateIpAddress = secStorageVm.getPrivateIpAddress() == null ? "N/A" : secStorageVm.getPrivateIpAddress(); | ||
| secStorageVmPodIdToDeployIn = secStorageVm.getPodIdToDeployIn(); | ||
| } |
There was a problem hiding this comment.
this is the exact same functionality as https://github.com/apache/cloudstack/pull/7140/files#diff-836e2e853d64b7a2f86c1add5aa76deea112788fe69c19a3c5eb2618d753ff2c lines 63-73. Can we make it a utiity method?
i.e. can we use VMInstanceVO instead of ConsoleProxyVO and SecondaryStorageVmVO and make it a member method of that class.
There was a problem hiding this comment.
Hey @DaanHoogland, yes this is replicate code from the ConsoleProxyAlertAdapter, but is it worth it to make a utility method out of it?
To make the code clear I would probably have to create a new object to store this info and use it during these methods. In the end I would probably end up with more lines of code and a whole new class just for this.
There was a problem hiding this comment.
@JoaoJandre , I am just bringing it to your attention. If you feel it isn't worth the trouble and you don't think it will be used more than considder my remark answerred.
Just one more opiniated remark; I think it is worth even if it doesn´t save lines of code to unify it. It makes clear to the reader that the same is happening, as future readers will not see the two peaces at the same time as we do now. This can also be solved by a comment of course.
|
@blueorangutan package |
|
@DaanHoogland a 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: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 5426 |
|
@blueorangutan test |
|
@rohityadavcloud a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-6018)
|
|
@rohityadavcloud , I am satisfied with just smoketests for this, would you agree? |
|
@JoaoJandre, could you look at the code smells as well?
|
|
@blueorangutan package |
|
@DaanHoogland a 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: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 5453 |
|
@blueorangutan test |
|
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-6040)
|
|
@blueorangutan test |
|
@rohityadavcloud a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian Build Failed (tid-6080) |
@DaanHoogland, as it is just a refactoring (to fix the bugs and vulnerabilities reported by the SonarCloud), I see no problem in adding this one to 4.18. We just have to guarantee that nothing broke with the refactoring; by the results of the tests and the code analysis, the changes seems OK; manual tests are welcome, though. |
3abfed4 to
a721520
Compare
|
SonarCloud Quality Gate failed. |
|
@blueorangutan package |
|
@DaanHoogland a 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: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 5561 |
|
@blueorangutan test |
|
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-6165)
|
|
@blueorangutan package |
|
@DaanHoogland a 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: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 5743 |
|
@blueorangutan test matrix |
|
@DaanHoogland a Trillian-Jenkins matrix job (centos7 mgmt + xenserver71, rocky8 mgmt + vmware67u3, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests |
|
Trillian test result (tid-6293)
|
|
Trillian test result (tid-6292)
|
|
Trillian test result (tid-6294)
|
|
This looks good to me, the kubernetes errors seem to be bugs in the tests assuming a state is returned when it is not in the assertions. If not that these are environmental. |
|
@DaanHoogland this was merged into 4.18 instead of main, is this correct? |
|
@JoaoJandre you will find it also if you look at the main git log . It was merged forward (as all fixes on prior release branches should) if you fix something now on 4.17, we will forward merge it on 4.18 and then on main. |
* 4.18: Fix PR 7131 bugs and vulnerabilities (#7140)
7 participants
Description
This PR fixes the bugs and vulnerabilities found by SonarCloud in PR #7131.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?