Make RedisInsight work with WithLifetime(...).#6425
Conversation
|
Can you add some tests for this? |
|
@eerhardt working on it. Turns out we don't have any tests for WithLifetime(...) so I'm having to tackle a few first of a kind problems:
|
|
OK for the time being I've done a bit of a hack to make sure we have test coverage here. See the introduction of
|
|
|
||
| foreach (var redisResource in redisInstances) | ||
| { | ||
| if (lookup is { } && lookup.Contains(redisResource.Name)) |
There was a problem hiding this comment.
Why are we deleting the servers that are already there, only to add them again?
Wouldn't the more appropriate approach be "if the server already exists, don't add it again" ?
There was a problem hiding this comment.
Because the credentials could have changed. Using the api/databases call we don't have a mechanism to check that the credentials are the same, so we just blow it away and recreate.
There was a problem hiding this comment.
It would be good to add this in a comment. I don't think this algorithm is intuitive.
There was a problem hiding this comment.
It would be good to add this in a comment. I don't think this algorithm is intuitive.
Did this ever happen?
eerhardt
left a comment
There was a problem hiding this comment.
The change looks fine. I'd just add some comments on why we are doing it this way.
|
/backport to release/9.0 |
|
Started backporting to release/9.0: https://github.com/dotnet/aspire/actions/runs/11490840397 |
Description
With this PR, when using
WithRedisInsight(...)the list of databases will be fetched from the RedisInsight instance and where they overlap with the names of the Redis Instances running already it will delete them so that the subsequent import won't result in duplicated entries.Fixes #6111
Checklist
<remarks />and<code />elements on your triple slash comments?Microsoft Reviewers: Open in CodeFlow