Skip to content

Conversation

@Alirexaa
Copy link
Contributor

@Alirexaa Alirexaa commented Mar 4, 2025

Description

The implementation is like #7599.

Contributes to #6155

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?

Copilot AI review requested due to automatic review settings March 4, 2025 18:21
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 4, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR adds support for specifying a password when configuring a Valkey resource and updates the related connection string generation and persistence annotation. Key changes include:

  • Introducing new overloads for AddValkey to allow optional password parameters.
  • Modifying the connection string assembly in ValkeyResource to include the password.
  • Updating tests and documentation to validate and demonstrate the new behavior.

Reviewed Changes

File Description
src/Aspire.Hosting.Valkey/ValkeyBuilderExtensions.cs Added overloads and extended configuration parameters, including password handling and enhanced documentation.
src/Aspire.Hosting.Valkey/ValkeyResource.cs Updated connection string logic to incorporate the password parameter and added a new constructor.
tests/Aspire.Hosting.Valkey.Tests/AddValkeyTests.cs Expanded tests to verify connection string formatting and persistence annotations with and without a password.

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/Aspire.Hosting.Valkey/ValkeyResource.cs:59

  • Ensure that PasswordParameter's string representation matches the expected placeholder format (e.g. '{myValkey-password.value}' or '{pass.value}'). If not, consider explicitly using the appropriate property from PasswordParameter.
builder.Append($",password={PasswordParameter}");

@mitchdenny
Copy link
Member

@eerhardt can I get your input on this? It seems reasonable to me. It looks like the bitnami packages do something similar to support setting passwords on their versions of the Valkey container image.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks again @Alirexaa!

@eerhardt eerhardt merged commit 9ee27d9 into dotnet:main Mar 5, 2025
73 checks passed
@Alirexaa Alirexaa deleted the valkey-password branch March 5, 2025 15:31
@github-actions github-actions bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Mar 10, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Apr 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants