Refactor rabbit service to return Results #552
Conversation
Codecov Report
@@ Coverage Diff @@
## main #552 +/- ##
==========================================
- Coverage 75.44% 75.35% -0.09%
==========================================
Files 47 47
Lines 2879 2857 -22
==========================================
- Hits 2172 2153 -19
+ Misses 569 566 -3
Partials 138 138
Continue to review full report at Codecov.
|
This should allow us to start creating policies instead of just queues and returning an aggregate result
| if !exchange.Ready { | ||
| logging.FromContext(ctx).Warnf("Exchange %q is not ready", exchange.Name) | ||
| MarkExchangeFailed(&b.Status, "ExchangeFailure", fmt.Sprintf("exchange %q is not ready", exchange.Name)) | ||
| return nil | ||
| } | ||
| args.Name = naming.BrokerExchangeName(b, true) | ||
| dlxExchange, err := r.rabbit.ReconcileExchange(ctx, args) |
There was a problem hiding this comment.
it's not clear that dlxExchange is not an Exchange anymore but a Result.
maybe other vars as well
There was a problem hiding this comment.
It still does represent the exchange, in that the Ready and Name fields are about the exchange, it's just a different struct than the CRD type. I don't really like calling the variable result, since that's just generic. I'm not crazy about exchangeResult either, I don't see a problem with it being called exchange still.
|
LGTM |
|
@deadtrickster: changing LGTM is restricted to collaborators In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
LGTM |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: benmoss, deadtrickster, ikvmw 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:
Approvers can indicate their approval by writing |
Changes
Resultobjects rather than the CRD/kind cleanup
Requested to be split out of #533 by @ikvmw