Skip to content
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

Add different modes for checking health on Azure Service Bus queues and subscriptions #1565

Merged
merged 47 commits into from
Jan 28, 2023

Conversation

marioleed
Copy link
Contributor

What this PR does / why we need it:
Adds AzureServiceBusOptions to enable different health check modes for Azure Service Bus queues and subscriptions.

Which issue(s) this PR fixes:

Please reference the issue this PR will close: #1556

Special notes for your reviewer:
This is a proposal aimed at minimal changes, hence only minor version is updated. I would have liked to add the setup action parameter before the name parameter, but that would introduce a breaking change and I feel it's not worth it at this point. However, if it's desired to re-arrange the parameters order (introducing a breaking change), then I'm fine with that too (the api would look cleaner IMHO).

I was considering an Enum for the different modes, though as you know, naming things is hard, so I used a bool rather than trying to come up with correct names for what the modes do.

Does this PR introduce a user-facing change?:

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Unit tests passing
  • End-to-end tests passing
  • Extended the documentation
  • Provided sample for the feature

@marioleed marioleed changed the title Feature/1556 claims Add different modes for checking health on Azure Service Bus queues and subscriptions Dec 15, 2022
@sungam3r
Copy link
Collaborator

This is a proposal aimed at minimal changes, hence only minor version is updated. I would have liked to add the setup action parameter before the name parameter, but that would introduce a breaking change and I feel it's not worth it at this point.

From API diff I see that you introduced binary breaking changes.

@sungam3r
Copy link
Collaborator

@marioleed I have a feeling that v7 packages will be published soon so I would do more breaking changes here, i.e. moving more properties into options class to simplify ctors.

Co-authored-by: Ivan Maximov <sungam3r@yandex.ru>
@marioleed
Copy link
Contributor Author

This is a proposal aimed at minimal changes, hence only minor version is updated. I would have liked to add the setup action parameter before the name parameter, but that would introduce a breaking change and I feel it's not worth it at this point.

From API diff I see that you introduced binary breaking changes.

Ah yes, I first set AzureServiceBusOptions? options = null in ctor's in AzureServiceBusQueueHealthCheck and AzureServiceBusSubscriptionHealthCheck, but then removed it, maybe I should use optional parameters anyway? Or do you want me to skip it for now as you mentioned v7 packages and breaking changes? What's your suggestion?

@sungam3r
Copy link
Collaborator

I'm waiting for review of #1574 from maintainers. After that my suggestion is - do any breaking change you want to improve public API surface/readability.

@sungam3r
Copy link
Collaborator

@marioleed #1574 in master, you can move on

@marioleed
Copy link
Contributor Author

@sungam3r I think I'm done. I guess I don't need to change version number as v7.0.0 already indicates a breaking change.

@marioleed marioleed requested a review from sungam3r December 23, 2022 10:47
@sungam3r sungam3r added the enhancement New feature or request label Dec 23, 2022
@sungam3r sungam3r added this to the V7 Release milestone Dec 23, 2022
@marioleed marioleed requested a review from sungam3r January 12, 2023 10:58
@sungam3r
Copy link
Collaborator

@marioleed Did you think about exposing extension metods with only options or Action<options> arguments?

@marioleed
Copy link
Contributor Author

marioleed commented Jan 28, 2023

@marioleed Did you think about exposing extension metods with only options or Action arguments?

@sungam3r Yes, I thought it was sufficient with Action<options>.

@codecov-commenter
Copy link

Codecov Report

Merging #1565 (b2030a4) into master (eeb2432) will increase coverage by 52.35%.
The diff coverage is 71.36%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##           master    #1565       +/-   ##
===========================================
+ Coverage   13.82%   66.18%   +52.35%     
===========================================
  Files           7      145      +138     
  Lines         188     3883     +3695     
  Branches       21      386      +365     
===========================================
+ Hits           26     2570     +2544     
- Misses        158     1226     +1068     
- Partials        4       87       +83     
Flag Coverage Δ
AzureServiceBus 56.14% <71.36%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...AzureServiceBus/AzureServiceBusQueueHealthCheck.cs 15.38% <11.11%> (ø)
...rviceBus/AzureServiceBusSubscriptionHealthCheck.cs 17.85% <15.00%> (ø)
...ecks.AzureServiceBus/AzureServiceBusHealthCheck.cs 50.00% <45.00%> (ø)
...AzureServiceBus/AzureServiceBusTopicHealthCheck.cs 28.57% <50.00%> (ø)
...Checks.AzureServiceBus/AzureEventHubHealthCheck.cs 43.18% <54.83%> (ø)
...ion/AzureServiceBusHealthCheckBuilderExtensions.cs 97.98% <95.41%> (ø)
...s/Configuration/AzureEventHubHealthCheckOptions.cs 100.00% <100.00%> (ø)
...Configuration/AzureServiceBusHealthCheckOptions.cs 100.00% <100.00%> (ø)
...guration/AzureServiceBusQueueHealthCheckOptions.cs 100.00% <100.00%> (ø)
...iceBusSubscriptionHealthCheckHealthCheckOptions.cs 100.00% <100.00%> (ø)
... and 138 more

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

@sungam3r
Copy link
Collaborator

Good, generic option argument is even better, completely forgot about this.

@sungam3r sungam3r merged commit d4def13 into Xabaril:master Jan 28, 2023
@sungam3r
Copy link
Collaborator

Thanks. Would you like me to publish previews ASAP? If not then I can do some other work here and publish all preview packages some time later.

@marioleed marioleed deleted the feature/1556-claims branch January 29, 2023 07:51
@marioleed
Copy link
Contributor Author

Thanks. Would you like me to publish previews ASAP? If not then I can do some other work here and publish all preview packages some time later.

There's no hurry 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
azure enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AzureServiceBus health check requires listen scope breaks security concerns
3 participants