-
Notifications
You must be signed in to change notification settings - Fork 684
Accept null value in Redis WithPassword to ensure password dosen't set in redis-server #9278
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
Conversation
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.
Pull Request Overview
This PR enables Redis resources to accept a null password value, ensuring that no password is exposed in the redis-server configuration. Key changes include updating tests to verify behavior when no password is provided, modifying RedisResource to accept a nullable parameter, and updating the RedisBuilderExtensions.WithPassword method to support null values.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
File | Description |
---|---|
tests/Aspire.Hosting.Redis.Tests/RedisFunctionalTests.cs | Adds a test case using WithPassword(null) for RedisInsight integration |
tests/Aspire.Hosting.Redis.Tests/AddRedisTests.cs | Adds and updates tests to verify manifest outputs and environment variable mappings based on password presence |
src/Aspire.Hosting.Redis/RedisResource.cs | Updates SetPassword to accept a nullable ParameterResource and removes the null check |
src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs | Modifies WithPassword to optionally accept a password resource and pass a nullable value |
Comments suppressed due to low confidence (2)
tests/Aspire.Hosting.Redis.Tests/AddRedisTests.cs:294
- Resource 'myredis2' is used for both redis2 and redis3, which may lead to conflicts when identifying distinct resources. Consider using a unique name for redis3.
var redis3 = builder.AddRedis("myredis2").WithRedisInsight().WithPassword(null);
tests/Aspire.Hosting.Redis.Tests/AddRedisTests.cs:130
- [nitpick] The test method names 'VerifyDefaultManifest' and 'VerifyWithoutPasswordManifest' are very similar. Consider renaming one to clearly differentiate the scenarios being tested.
[Fact] public async Task VerifyWithoutPasswordManifest()
Somehow passing null feels dirtier than another method but idk. |
You mean a new method like |
Yea |
not a hill I will die on |
I think I'm fine with a single method that takes null. It doesn't feel terrible to me. |
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.
Lets get this in.
Co-authored-by: Eric Erhardt <[email protected]>
Co-authored-by: Eric Erhardt <[email protected]>
Test failure is unrelated. |
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.
LGTM
Don't forget to backport. |
/backport to release/9.3 |
Started backporting to release/9.3: https://github.com/dotnet/aspire/actions/runs/14999610625 |
from api review: #8736 (comment)
fixes #8897
/cc @eerhardt @davidfowl