-
Notifications
You must be signed in to change notification settings - Fork 1.2k
api,ui: fix empty source cidr value for firewall rule #10208
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
api,ui: fix empty source cidr value for firewall rule #10208
Conversation
Fixes apache#10002 Signed-off-by: Abhishek Kumar <[email protected]>
af70946 to
4762115
Compare
|
@shwstppr a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress. |
|
UI build failed: ✖️ |
Signed-off-by: Abhishek Kumar <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 4.19 #10208 +/- ##
============================================
+ Coverage 15.13% 15.14% +0.01%
- Complexity 11274 11285 +11
============================================
Files 5408 5408
Lines 473974 473769 -205
Branches 57813 57813
============================================
+ Hits 71730 71768 +38
+ Misses 394227 393982 -245
- Partials 8017 8019 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@shwstppr a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress. |
|
UI build: ✔️ |
abh1sar
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.
LGTM.
p.s. what would be the behaviour if the same was done using api (Passing empty string cidr)? If it is the same, can we sanitise the input or throw some error from the api?
Signed-off-by: Abhishek Kumar <[email protected]>
|
@abh1sar API was allowing empty cidr as it takes List as input, I've added a check @blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12164 |
api/src/main/java/org/apache/cloudstack/api/command/user/firewall/CreateFirewallRuleCmd.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Abhishek Kumar <[email protected]>
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12184 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian Build Failed (tid-12176) |
weizhouapache
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
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[LL]Trillian test result (tid-7066)
|
|
[SF] Trillian test result (tid-12185)
|
DaanHoogland
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, tested in a lab env (also unit tests provided
Signed-off-by: Abhishek Kumar <[email protected]>
Description
Fixes #10002
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?