-
Notifications
You must be signed in to change notification settings - Fork 59
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
Update sample to dotnet 8, use simplified API, simplify authentication #62
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Richard87 <[email protected]>
@@ -37,11 +37,11 @@ spec: | |||
- name: order-processor | |||
image: ghcr.io/kedacore/sample-dotnet-worker-servicebus-queue:latest | |||
env: | |||
- name: KEDA_SERVICEBUS_AUTH_MODE | |||
- name: OrderQueueOptions__AuthMode |
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.
Let's revert all of these renames and stick with current names
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 would prefer to, but I don't know how when using DotNets option pattern., do you have any advice?
I could revert the configuration integration and just parse it myself in the options class, if that is preferable?
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.
Let's revert it for now and keep it simple
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.
Can I suggest an upgrade?
In the fork we have removed all auth options, and only retained AzureDefaultCredentials... What do you think about the same here, but also only checking if KEDA_SERVICEBUS_CONNECTION_STRING
is used, and if so, only using that?
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.
btw, that was my last question, if you still want to retain the original features, I'll get right on it :)
public enum AuthenticationMode | ||
{ | ||
ConnectionString, | ||
ServicePrinciple, | ||
PodIdentity, | ||
WorkloadIdentity, | ||
AzureDefaultCredential, | ||
} | ||
public class OrderQueueOptions |
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.
Let's align with the current way of working and create a file per class please
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.
Fixed :)
public record Customer(string FirstName, string LastName); | ||
public record Order(string Id, int Amount, string ArticleNumber, Customer Customer); | ||
public record QueueStatus(long MessageCount); |
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.
Let's align with the current way of working and create a file per class please
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.
Fixed :)
I think some of the readme might be outdated, but its been tested with ConnectionString and AzureDefaultCredential.
AddServiceBusClientExtension
and used it in all three librariesProgram.cs
with new minimalist api style