-
Notifications
You must be signed in to change notification settings - Fork 574
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
Broker class based defaults #7631
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Leo Li <leoli@redhat.com>
Skipping CI for Draft Pull Request. |
Signed-off-by: Leo Li <leoli@redhat.com>
c4a2ff3
to
662c214
Compare
Signed-off-by: Leo Li <leoli@redhat.com>
Signed-off-by: Leo Li <leoli@redhat.com>
Signed-off-by: Leo Li <leoli@redhat.com>
Signed-off-by: Leo Li <leoli@redhat.com>
Signed-off-by: Leo Li <leoli@redhat.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7631 +/- ##
==========================================
+ Coverage 69.22% 69.48% +0.26%
==========================================
Files 339 344 +5
Lines 19494 15921 -3573
==========================================
- Hits 13494 11063 -2431
+ Misses 5337 4183 -1154
- Partials 663 675 +12 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Leo Li <leoli@redhat.com>
Signed-off-by: Leo Li <leoli@redhat.com>
return d.ClusterDefaultConfig.BrokerConfig, nil | ||
} | ||
|
||
if config, ok := d.ClusterDefaultConfig.BrokerClasses[brokerClassName]; ok && config != nil { |
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.
shouldn't we check before, if d.ClusterDefaultConfig.BrokerClass == brokerClassName
and if this is the case return d.ClusterDefaultConfig.BrokerConfig
, as you wrote in https://docs.google.com/document/d/1RKij-DYPmcbCTHF26hkXdrtWHqrksA-Fo5qYLRP7xVA/edit
# The brokerClasses contain the configmap, delivery spec for other brokerClasses.
# The one specified above is NOT included.
# Reason: to maintain compatibility, we are not removing those old fields. And it is not
# necessary to add it again here under brokerClasses.
Otherwise d.ClusterDefaultConfig.BrokerConfig
does not take preference over d.ClusterDefaultConfig.BrokerClasses[]
(what I meant in #7631 (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.
I think the logic here is correct, prob I didn't explain clearly in the Doc, sorry about that! @creydr
if d.ClusterDefaultConfig.BrokerClass == brokerClassName
, then we will directly use d.ClusterDefaultConfig.BrokerConfig
. And this brokerClass shouldn't appear in d.ClusterDefaultConfig.BrokerClasses
.
When I was referring to:
The one specified above is NOT included.
It means the brokerConfig for d.ClusterDefaultConfig.BrokerClass
will not be included in the d.ClusterDefaultConfig.BrokerClasses
pkg/apis/config/defaults_test.go
Outdated
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 we change this for easier understanding and verification to a table-test? So that we specify for each case the input and expected output? Similar to what we did in TestDefaultsConfiguration()
That way we could also easily add a case for all of the cases described in https://docs.google.com/document/d/1RKij-DYPmcbCTHF26hkXdrtWHqrksA-Fo5qYLRP7xVA/edit and verify if it works as expected
Co-authored-by: Christoph Stäbler <cstabler@redhat.com>
Signed-off-by: Leo Li <leoli@redhat.com>
…e cluster Signed-off-by: Leo Li <leoli@redhat.com>
Signed-off-by: Leo Li <leoli@redhat.com>
/hold currently trying to manually test the functionality |
Manually creating the broker and test it, some problem occurs: the config from the brokerClasses cannot be used. Currently investigating |
This Pull Request is stale because it has been open for 90 days with |
Hey @Leo6Leo anything I can help with on this PR? |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Leo6Leo The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/remove-lifecycle stale Resume working on the issue |
…ble instead Signed-off-by: Leo Li <leoli@redhat.com>
…r has value, it will pass. So this commit fix that problem. Signed-off-by: Leo Li <leoli@redhat.com>
Signed-off-by: Leo Li <leoli@redhat.com>
Signed-off-by: Leo Li <leoli@redhat.com>
Signed-off-by: Leo Li <leoli@redhat.com>
Signed-off-by: Leo Li <leoli@redhat.com>
Currently, the table unit test
Note: this is where clusterReference.Namespace is set. @creydr @pierDipi @Cali0707 Do you guys know how to run the tests in the eventing-rabbitmq repo locally with the changes I have made in this PR? |
Not sure, if there is a simple way, but you can use your branch to eventing-core in eventing-rabbitsmqs go.mod file via a replace directive. E.g. something like this:
and then run |
Fixes #5992
Proposed Changes
Pre-review Checklist
Release Note
Docs