-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Update SignalR Redis tests to use internal Docker Hub mirror #62936
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
Docker Hub is prone to rate limiting. To prevent this in dotnet builds we should use our internal container registry.
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 updates the SignalR Redis test infrastructure to use an internal Azure Container Registry mirror instead of Docker Hub to avoid rate limiting issues during builds.
- Updates the Redis Docker image source from Docker Hub to an internal Azure Container Registry mirror
- Changes the Docker container name to reflect the specific Redis version being used
@@ -16,7 +16,8 @@ public class Docker | |||
{ | |||
private static readonly string _exeSuffix = OperatingSystem.IsWindows() ? ".exe" : string.Empty; | |||
|
|||
private static readonly string _dockerContainerName = "redisTestContainer"; | |||
private static readonly string _redisImageName = "dotnetdhmirror-f8bzbjakh8cga6ab.azurecr.io/library/redis:7.4"; |
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.
The hardcoded Azure Container Registry URL makes the code less maintainable. Consider extracting this to a configuration value or environment variable to allow easier updates when the registry URL changes.
private static readonly string _redisImageName = "dotnetdhmirror-f8bzbjakh8cga6ab.azurecr.io/library/redis:7.4"; | |
private static readonly string _redisImageName = Environment.GetEnvironmentVariable("REDIS_IMAGE_NAME") | |
?? "dotnetdhmirror-f8bzbjakh8cga6ab.azurecr.io/library/redis:7.4"; |
Copilot uses AI. Check for mistakes.
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.
I don't think we need this, this is a good suggestion though
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.
maybe also extract the ACR host URL into a separate property
@@ -16,7 +16,8 @@ public class Docker | |||
{ | |||
private static readonly string _exeSuffix = OperatingSystem.IsWindows() ? ".exe" : string.Empty; | |||
|
|||
private static readonly string _dockerContainerName = "redisTestContainer"; | |||
private static readonly string _redisImageName = "dotnetdhmirror-f8bzbjakh8cga6ab.azurecr.io/library/redis:7.4"; | |||
private static readonly string _dockerContainerName = "redisTestContainer74"; |
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.
The container name hardcodes the Redis version '74' which will need manual updates when the Redis version changes. Consider deriving the container name from the image version or using a more generic naming approach.
private static readonly string _dockerContainerName = "redisTestContainer74"; | |
private static readonly string _redisVersion = _redisImageName.Split(':')[1]; // Extract version from image name | |
private static readonly string _dockerContainerName = $"redisTestContainer{_redisVersion.Replace('.', '_')}"; // Replace '.' with '_' for valid container name |
Copilot uses AI. Check for mistakes.
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.
I don't think this is worth the complexity
Docker Hub is prone to rate limiting. To prevent this in dotnet builds we should use our internal container registry.
c.f. https://dev.azure.com/dnceng/internal/_wiki/wikis/DNCEng%20Services%20Wiki/1354/ACR-for-Docker-Hub-mirroring