Sugar controller doesn't enforce Broker class

[gab-satchi]

• 🐛 Fixes issue where sugar controller was forcing Broker class.

Pre-review Checklist

• At least 80% unit test coverage
• E2E tests for any new behavior
• Docs PR for any user-facing impact
• Spec PR for any new API feature
• Conformance test for any change to the spec

To reproduce:

• Update the defaults ConfigMap config-br-defaults to use RabbitMQBroker along with an appropriate broker config.
• Enable sugar controller
• Create a new namespace or trigger with the appropriate label

Expected:

• default broker is created with configuration and class matching the defaults config map

Actual:

• default broker is created with the RabbitMQ broker config but with the class still set to MTChannelBasedBroker

Fixes issue with sugar controller always setting the broker class to MTChannelBasedBroker instead of using the defaults ConfigMap

Relies on this pkg PR to be merged and integrated for tests to pass

/hold until pkg merge

[matzew]

One thing that I noticed is these hard-coded defaults:
https://github.com/knative/eventing/blob/main/pkg/reconciler/sugar/resources/broker.go#L35

Should we actually honor what's configured as default broker - instead of the above assumption?

[gab-satchi]

I think the hardcoded default may have been an oversight. The hard coded default means the sugar controller will not work with any broker class other than the MTBroker.

[codecov]

Codecov Report

Base: 82.09% // Head: 81.94% // Decreases project coverage by -0.15% ⚠️

Coverage data is based on head (1e06cc5) compared to base (db235e7).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6500      +/-   ##
==========================================
- Coverage   82.09%   81.94%   -0.16%     
==========================================
  Files         235      235              
  Lines       11714    11737      +23     
==========================================
+ Hits         9617     9618       +1     
- Misses       1628     1646      +18     
- Partials      469      473       +4     

Impacted Files	Coverage Δ
pkg/reconciler/sugar/resources/broker.go	100.00% <100.00%> (ø)
pkg/channel/message_receiver.go	64.22% <0.00%> (-9.82%)	⬇️
pkg/scheduler/statefulset/scheduler.go	79.13% <0.00%> (-2.80%)	⬇️
pkg/channel/message_dispatcher.go	81.15% <0.00%> (-0.90%)	⬇️
pkg/scheduler/scheduler.go	100.00% <0.00%> (ø)
...labilityzonepriority/availability_zone_priority.go	82.60% <0.00%> (+11.18%)	⬆️
...labilitynodepriority/availability_node_priority.go	81.39% <0.00%> (+11.18%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[gab-satchi]

@pierDipi, The PR for the bug I was fixing. It now uses a defaulter reactor instead of modifying pkg code.

[gab-satchi]

/hold cancel

[pierDipi]

Thanks!

/lgtm
/approve

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gab-satchi, pierDipi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [pierDipi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment