Skip to content

Conversation

Alirexaa
Copy link
Contributor

@Alirexaa Alirexaa commented Apr 3, 2025

Description

This pull request includes significant changes to the Aspire.Hosting.Redis library, focusing on simplifying the WithRedisInsight method, updating the RedisInsight image tag, and improving test coverage. The most important changes are grouped into codebase simplification, image tag updates, and test improvements.

Codebase Simplification:

Image Tag Updates:

Test Improvements:

Fixes #8506
Fixes #7291
Fixes #6099
Fixes #7176

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?

@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 3, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 3, 2025
@Alirexaa Alirexaa changed the title Change Redis Insights to use environments variable for preconfigured database connections Change Redis Insights to use environment variables for preconfigured database connections Apr 3, 2025
@Alirexaa
Copy link
Contributor Author

Alirexaa commented Apr 3, 2025

@eerhardt, could you import the new Redis Insights tag to netaspireci.azurecr.io? Some tests fail because of that.

@Alirexaa Alirexaa marked this pull request as ready for review April 3, 2025 17:37
@Copilot Copilot AI review requested due to automatic review settings April 3, 2025 17:37
Copy link
Contributor

@Copilot 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.

Pull Request Overview

This PR refactors the RedisInsight integration in the Aspire.Hosting.Redis library to simplify the WithRedisInsight method, update the RedisInsight image tag, and improve test coverage by replacing outdated tests with a dedicated test for environment variables.

  • Simplified the WithRedisInsight method by removing the ImportRedisDatabases logic and event subscriptions
  • Updated the RedisInsight image tag from 2.66 to 2.68
  • Enhanced tests with a new test verifying correct environment variable configuration and removed quarantined tests

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 Removed quarantined tests to streamline the RedisInsight test suite
tests/Aspire.Hosting.Redis.Tests/AddRedisTests.cs Added new test to verify RedisInsight environment variable configuration
src/Aspire.Hosting.Redis/RedisContainerImageTags.cs Updated RedisInsight image tag to 2.68
src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs Simplified the WithRedisInsight method and refactored environment setup
Comments suppressed due to low confidence (2)

src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs:251

  • Ensure that each redis instance's PrimaryEndpoint is allocated before accessing its TargetPort to avoid potential null reference issues. Consider adding a conditional check before adding the environment variable.
context.EnvironmentVariables.Add("RI_REDIS_PORT" + counter, redisInstance.PrimaryEndpoint.TargetPort!.Value);

src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs:244

  • [nitpick] If a deterministic order of redisInstances is important for consistent environment variable names, consider sorting the redisInstances collection before iterating.
var counter = 1;

@eerhardt
Copy link
Member

eerhardt commented Apr 3, 2025

@eerhardt, could you import the new Redis Insights tag to netaspireci.azurecr.io? Some tests fail because of that.

Done.

@eerhardt
Copy link
Member

eerhardt commented Apr 3, 2025

This also fixes #7291.

@Alirexaa
Copy link
Contributor Author

Alirexaa commented Apr 3, 2025

@eerhardt, After this change, the EULA and Privacy Settings dialog will open after every run unless users user WithDataVolume

@eerhardt
Copy link
Member

eerhardt commented Apr 3, 2025

@eerhardt, After this change, the EULA and Privacy Settings dialog will open after every run unless users user WithDataVolume

Yep - that is intentional / by design. It isn't necessarily an "ideal" user experience, but it isn't Aspire's to decision to make. (note the user should be able to make the redis and redis-insights persistent to not have to accept the EULA as well). Check out

@Alirexaa
Copy link
Contributor Author

Alirexaa commented Apr 3, 2025

I updated the PR description. This PR fixes four issues. See the description for those.

- Remove IsAllocated check
- Minor code clean up.
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.

This is great work, @Alirexaa! Thanks for the contribution.

LTGM.

@eerhardt eerhardt enabled auto-merge (squash) April 4, 2025 15:38
@eerhardt eerhardt merged commit c09db91 into dotnet:main Apr 4, 2025
174 checks passed
@Alirexaa Alirexaa deleted the redis-insights branch April 4, 2025 17:06
@github-actions github-actions bot locked and limited conversation to collaborators May 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community-contribution Indicates that the PR has been added by a community member needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
3 participants