Accept null value in Redis WithPassword to ensure password dosen't set in redis-server#9278
Conversation
There was a problem hiding this comment.
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. |
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
|
Test failure is unrelated. |
|
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