-
Notifications
You must be signed in to change notification settings - Fork 1.2k
List only untagged offerings for Shared networks when tag isn't passed #10320
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 4.19 #10320 +/- ##
===========================================
Coverage 15.16% 15.16%
- Complexity 11302 11333 +31
===========================================
Files 5408 5409 +1
Lines 473931 476911 +2980
Branches 57844 58763 +919
===========================================
+ Hits 71866 72332 +466
- Misses 394034 396507 +2473
- Partials 8031 8072 +41
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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 looks good, but not sure if this is functionally what we want.
cc @rajujith
|
@Pearl1594 After the fix the listing does list all networking even though a tag is passed? also shouldn't it list all network offerings when a tag is not passed? |
|
to be frank, I am a bit confused what is expected |
|
@weizhouapache I've added the isTagged=false if no tags are present on the physical network. |
thanks @Pearl1594 code lgtm |
| sc.addAnd("tags", SearchCriteria.Op.EQ, tags); | ||
| } | ||
| } | ||
| // else { |
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.
remove this ?
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
Outdated
Show resolved
Hide resolved
|
|
@rajujith , can you retest? |
|
@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 |
|
@rajujith 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 12372 |
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 against a lab env



Description
This PR attempts to improve #10222.

Before fix:
for default untagged physical network - all shared network offerings are listed, including the tagged ones:
for Physical network with tag: phy2 - all shared offering are listed - default and tagged:

After fix:

for default untagged physical network - only untagged offerings are listed:
for Physical network with tag: phy2 - all shared offering are listed - default and tagged:

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?