-
Notifications
You must be signed in to change notification settings - Fork 61
Separate Registry Endpoints from Authentication in PublishConfig #1945
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
base: main
Are you sure you want to change the base?
Conversation
…ame service connection
CopyAcrImagesCommand was passing srcResourceId to the wrapper method, but CopyImagesCommand.ImportImageAsync ignored it and passed null for srcRegistryName to the service. This caused CopyImageService to set ResourceId to null, resulting in Azure API 400 errors. Fix by passing srcRegistryName instead, allowing CopyImageService to look up the ResourceId from the registry name as designed.
Azure ACR import only accepts one source identifier. Set ResourceId for ACR-to-ACR imports, or RegistryAddress for external registries, not both.
3613bfa to
2ca4696
Compare
These CLI options (--acr-subscription, --acr-resource-group) were defined but never used by BuildCommand. Registry subscription/resource group info is now provided per-registry via PublishConfiguration.
fe8ffe3 to
bee0284
Compare
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 refactors PublishConfiguration to separate registry endpoint addresses from authentication details, making it easier to share credentials across registries and support non-ACR registries.
Changes:
- Introduced
RegistryEndpointandRegistryAuthenticationtypes to separate concerns - Changed
PublishConfigurationschema to useRegistryEndpointfor registry references and a list ofRegistryAuthenticationfor credential lookup - Removed subscription and resource group CLI arguments from commands, moving this data to the publish configuration
- Updated all consumers to use new lookup methods (
FindRegistryAuthentication,GetRegistryResource)
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| RegistryEndpoint.cs | New record type holding registry server address only |
| RegistryAuthentication.cs | New record type for authentication and Azure metadata |
| RegistryExtensions.cs | Extension methods for registry operations |
| PublishConfiguration.cs | Updated to use new types and lookup structure |
| ConfigurationExtensions.cs | Added lookup methods for authentication and resource IDs |
| RegistryResolver.cs | Updated to use RegistryAuthentication instead of RegistryConfiguration |
| RegistryManifestClientFactory.cs | Updated to use new authentication lookup |
| RegistryCredentialsProvider.cs | Updated method signature to accept RegistryAuthentication |
| CopyImageService.cs | Moved resource ID creation from parameters to internal lookup |
| CopyImagesOptions.cs | Removed subscription and resource group properties |
| CopyImagesCommand.cs | Simplified with primary constructor, removed subscription/resource group parameters |
| BuildOptions.cs | Removed subscription and resource group CLI options |
| BuildCommand.cs | Updated to use simplified ImportImageAsync signature |
| AcrContentClientFactory.cs | Updated to use new authentication lookup |
| AcrClientFactory.cs | Updated to use new authentication lookup |
| PublishConfigurationBindingTests.cs | New test file for configuration binding validation |
| Various test files | Updated to remove subscription/resource group setup and verification |
| public string? Server { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// The Azure DevOps service connection for authenticating to this registry. |
Copilot
AI
Feb 2, 2026
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 comment refers to 'Azure DevOps service connection' but the broader context suggests this could be used with other authentication mechanisms. Consider updating the comment to be more general or clarify that Azure DevOps is just one example.
| /// The Azure DevOps service connection for authenticating to this registry. | |
| /// The service connection used for authenticating to this registry (for example, an Azure DevOps service connection). |
| var resourceId = | ||
| ContainerRegistryResource.CreateResourceIdentifier( | ||
| subscriptionId: subscription, | ||
| resourceGroupName: resourceGroup, | ||
| registryName: Acr.Parse(registry).Name); |
Copilot
AI
Feb 2, 2026
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 method calls Acr.Parse(registry) to extract the registry name, but Acr.Parse() was already called earlier in FindRegistryAuthentication (line 35). Consider passing the already-parsed Acr object to avoid redundant parsing.
| Acr destAcr = Acr.Parse(destAcrName); | ||
|
|
||
| // Azure ACR import only supports one source identifier. Use ResourceId for ACR-to-ACR | ||
| // imports (same tenant), or RegistryAddress for external registries. |
Copilot
AI
Feb 2, 2026
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 comment states 'same tenant' but the code logic doesn't verify tenant matching. Consider clarifying whether tenant validation happens elsewhere or if this assumption should be documented as a requirement.
| // imports (same tenant), or RegistryAddress for external registries. | |
| // imports when the source and destination registries are expected to be in the same tenant | |
| // (this method does not validate tenant matching), or RegistryAddress for external registries. |
Fixes #1914.
Problem
The current
PublishConfigurationtightly couples registry endpoints with authentication details. EachRegistryConfigurationembeds its ownServiceConnection,ResourceGroup, andSubscription, making it difficult to:Changes
Refactored
PublishConfigurationto separate concerns:New types:
RegistryEndpoint- Holds only the registry server addressRegistryAuthentication- HoldsServer+ServiceConnection+ ACR metadata (ResourceGroup,Subscription)New schema:
{ "PublishConfiguration": { "BuildRegistry": { "Server": "build.azurecr.io" }, "PublishRegistry": { "Server": "publish.azurecr.io" }, "InternalMirrorRegistry": { "Server": "internal-mirror.azurecr.io" }, "PublicMirrorRegistry": { "Server": "public-mirror.azurecr.io" }, "RegistryAuthentication": [ { "Server": "build.azurecr.io", "ServiceConnection": { "Name": "...", "Id": "...", "TenantId": "...", "ClientId": "..." }, "ResourceGroup": "rg-build", "Subscription": "sub-build" }, { "Server": "publish.azurecr.io", "ServiceConnection": { "Name": "...", "Id": "...", "TenantId": "...", "ClientId": "..." }, "ResourceGroup": "rg-publish", "Subscription": "sub-publish" } ] } }Multiple registries can now share authentication by referencing the same server in
RegistryAuthentication. The authentication is looked up by server address usingFindRegistryAuthentication().Registry Endpoints Added:
BuildRegistry- Images are built and pushed here before testing and publishingPublishRegistry- Images are copied from BuildRegistry to here during publishingInternalMirrorRegistry- External image dependencies are mirrored herePublicMirrorRegistry- External images are mirrored here with anonymous pull access for public PR validationFiles changed:
RegistryEndpoint.cs- Registry endpoint record with onlyServerpropertyRegistryAuthentication.cs- Authentication record withServer,ServiceConnection,ResourceGroup,SubscriptionRegistryExtensions.cs- Extension methods for registry operationsPublishConfiguration.cs- Now usesRegistryEndpointfor registry references andList<RegistryAuthentication>for auth lookupConfigurationExtensions.cs- AddedFindRegistryAuthentication()andGetRegistryResource()methodsPublishConfigurationBindingTests.cs- Tests for config binding from JSONFindRegistryAuthentication()Breaking change: appsettings.json/PublishConfiguration schema has changed.
RegistryAuthenticationis now a list keyed byServerinstead of a dictionary.