Should IAzureComputeEnvironmentResource.ContainerRegistry be obsoleted in favor of ContainerRegistryReferenceAnnotation?
- Dominant language
- C#
- Stars
- 6.3k
- Forks
- 991
- Avg merge
- 2d 12h
- Merged PRs (30d)
- 201
Description
## Question
Should `Aspire.Hosting.Azure.IAzureComputeEnvironmentResource.ContainerRegistry` be marked `[Obsolete]` (or removed from the interface and replaced with an annotation-based public helper)?
Today the interface declares a default-implementation property:
```csharp
public interface IAzureComputeEnvironmentResource : IComputeEnvironmentResource, IResource
{
IAzureContainerRegistryResource? ContainerRegistry => null;
}
```
In practice, the runtime resolution path that every compute environment uses to find its container registry is the annotation-based one — `TryGetLastAnnotation` on the environment resource. The same pattern appears in:
- `AzureContainerAppEnvironmentResource.GetContainerRegistry`
- `AzureAppServiceEnvironmentExtensions`
- `AzureContainerAppExtensions`
- `KubernetesEnvironmentResource`
- `DockerComposeEnvironmentResource`
- `Aspire.Hosting.Foundry/Project/ProjectResource`
And `AzureContainerRegistryExtensions.AddAzureContainerRegistry` stamps a `RegistryTargetAnnotation` on every resource via `OnBeforeStart`. So the annotation is the canonical source of truth at runtime.
The only public consumer that reads the interface property directly is `AzureContainerRegistryExtensions.GetAzureContainerRegistry()`:
```csharp
public static IResourceBuilder GetAzureContainerRegistry(
this IResourceBuilder builder)
where T : IResource, IAzureComputeEnvironmentResource
{
var containerRegistry = builder.Resource.ContainerRegistry
?? throw new InvalidOperationException(...);
...
}
```
That call works for `AzureContainerAppEnvironmentResource` (which overrides `ContainerRegistry`) but throws for `AzureKubernetesEnvironmentResource`, because AKS doesn't override the interface property — it only sets the annotation. The AKS deploy path itself doesn't care, because it goes through the annotation; but `GetAzureContainerRegistry()` and any downstream consumer (e.g. [devdiv-microsoft/aspire-1p#238](https://github.com/devdiv-microsoft/aspire-1p/issues/238)) that reads the property directly is left in a different state from the annotation.
That parallelism — a public property and an annotation that are supposed to mean the same thing, but only sometimes agree — is what's prompting the question.
## Options
A few directions worth discussing:
1. **Annotation-first, keep the property.** Update `GetAzureContainerRegistry()` to read `ContainerRegistryReferenceAnnotation` first, then fall back to `IAzureComputeEnvironmentResource.ContainerRegistry`. Leave the interface property as-is; document that the annotation is canonical.
2. **Mark the interface property `[Obsolete]`.** Same behavior change to `GetAzureContainerRegistry()` as in (1), plus a soft-deprecation on `IAzureComputeEnvironmentResource.ContainerRegistry` pointing folks at the extension/annotation. The default-interface implementation can keep returning `null` indefinitely.
3. **Remove the property from the interface in a future major version.** After (2), once consumers have migrated, drop the property entirely and have `GetAzureContainerRegistry()` be the only public way to ask "what registry does this compute environment publish to?".
4. **Status quo + override on AKS.** Override `ContainerRegistry` on `AzureKubernetesEnvironmentResource` to return `DefaultContainerRegistry ?? `, so the interface property and the annotation always agree. This keeps the interface property as the canonical answer but doesn't address the broader "two sources of truth" smell.
I don't have a strong opinion on which is right; mainly raising this so the team can decide whether the property should stay first-class or be obsoleted in favor of the annotation-based pattern that the runtime already uses everywhere.
## Repro / related
[devdiv-microsoft/aspire-1p#238](https://github.com/devdiv-microsoft/aspire-1p/issues/238) hits the property-vs-annotation mismatch on AKS today.
Contributor guide
Research direction
Start with AzureContainerRegistryExtensions.GetAzureContainerRegistry(), IAzureComputeEnvironmentResource.ContainerRegistry, and the ContainerRegistryReferenceAnnotation lookup. Compare the behavior across AzureContainerAppEnvironmentResource, AzureKubernetesEnvironmentResource, and the other listed environment resources, including the linked AKS mismatch. Done means the team has selected and implemented one consistent public API direction with aligned documentation and tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- azure, csharp
- Domain
- backend-api-design, cloud
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Needs clarification
- Newbie friendliness
- 35/100