Add optional parameters to the ServiceCollection extensions#27
Add optional parameters to the ServiceCollection extensions#27ncipollina merged 2 commits intoLayeredCraft:mainfrom
Conversation
* Add optional parameters to the ServiceCollection extensions to configure dynamodb client. Fix warnings in unit tests by passing test cancellation token * add tests
| public static IServiceCollection AddDynamoDbDistributedLock(this IServiceCollection services, | ||
| IConfiguration configuration, | ||
| string sectionName = DynamoDbLockOptions.DynamoDbLockSettings) | ||
| string sectionName = DynamoDbLockOptions.DynamoDbLockSettings, string? awsOptionsSectionName = null) |
There was a problem hiding this comment.
Adding these optional parameters breaks binary compatibility, but because this has an existing optional param of the same type, I couldn't think of a way to do the trick of manually making another set of methods, because they are ambiguous and one gets hidden.
I think this is probably OK, since in my experience binary breaks are not as big of a deal in modern dotnet + nuget, and if you did hit this by copying some old dll, you would see it immediately in startup.
| return Task.FromResult(expectedResult); | ||
| }, | ||
| _ => true); | ||
| }, _ => true, TestContext.Current.CancellationToken); |
There was a problem hiding this comment.
xunit analyzer was warning about not using a cancellation token here, and recommended this one, so that if you cancel tests that hang they will have a better chance of failing fast and gracefully. This does slightly change the code paths being hit, but seems worth it.
| { | ||
| // Arrange | ||
| var services = new ServiceCollection(); | ||
| var awsOptions = new AWSOptions |
There was a problem hiding this comment.
The unit tests only cover the overload that passes in the object, not the config section, because in the other tests you override the dynamodb service in order to bypass credential resolution. I couldn't figure out a good way to make all that work outside of setting fake aws env vars, and if I did the same thing of registering a substitute, it wouldn't actually test anything.
So to increase coverage you could set aws access keys and secret env vars, but that makes me feel gross in tests so I didn't.
There was a problem hiding this comment.
Pull Request Overview
This PR adds optional parameters to ServiceCollection extensions for configuring the DynamoDB client with custom AWS options, enabling scenarios like using custom service URLs for private links or LocalStack testing. Additionally, it fixes xUnit analyzer warnings by passing cancellation tokens to test methods.
- Add optional
AWSOptionsparameter to service registration methods - Add optional
awsOptionsSectionNameparameter for configuration-based setup - Fix xUnit analyzer warnings by adding cancellation token parameters to test method calls
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/DynamoDb.DistributedLock/Extensions/ServiceCollectionExtensions.cs | Add optional AWSOptions parameters to both extension methods for DynamoDB client configuration |
| src/DynamoDb.DistributedLock/DynamoDbLockOptions.cs | Add XML documentation comment for the DynamoDbLockSettings constant |
| test/DynamoDb.DistributedLock.Tests/Extensions/ServiceCollectionExtensionsTests.cs | Add test case for new AWSOptions functionality |
| test/DynamoDb.DistributedLock.Tests/Retry/RetryIntegrationTests.cs | Add cancellation token parameters to test method calls |
| test/DynamoDb.DistributedLock.Tests/Retry/ExponentialBackoffRetryPolicyTests.cs | Add cancellation token parameters and clean up lambda formatting |
| README.md | Add documentation examples for the new AWSOptions configuration capabilities |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
ncipollina
left a comment
ncipollina
left a comment
There was a problem hiding this comment.
🚀The change looks good. The only ask I have is that you update the VersionPrefix in Directory.Build.props to 1.1.2.
|
@ncipollina ah, I looked in the project file and didn't see that. Didn't know if your actions did magic.
|
|
@all-contributors add @TylerReid for code |
|
I've put up a pull request to add @TylerReid! 🎉 |
2 participants
🚀 Pull Request
📋 Summary
This allows users to configure the DynamoDB client in more ways. This can be helpful in scenarios such as needing to set a custom ServiceURL for private link inside aws, or to use localstack for testing.
✅ Checklist
🧪 Related Issues or PRs
Closes #25
💬 Notes for Reviewers
This contains a binary breaking change due to the optional parameters.
I went ahead and fixed some xunit analyzer warnings about cancellation tokens in the tests. It was so close to no warnings in the build I couldn't resist 🙂