Simplify AzureProvisioner and make it testable by removing unnecessary abstraction layers

[Copilot]

This PR simplifies the AzureProvisioner by removing unnecessary abstraction layers that were built for multiple provisioners but only had one implementation (BicepProvisioner). The changes make the code more maintainable and testable while preserving all existing functionality.

Changes Made

Removed Unnecessary Abstractions

• Deleted IAzureResourceProvisioner interface - No longer needed since only one implementation existed
• Deleted AzureResourceProvisioner<T> base class - Unnecessary generic abstraction
• Deleted BicepProvisioner class - Integrated its functionality directly into AzureProvisioner
• Removed provisioner selection logic - The SelectProvisioner method that always returned BicepProvisioner
• Removed AddAzureProvisioner<TResource, TProvisioner> extension method - No longer needed

Integrated BicepProvisioner into AzureProvisioner

• Moved all Bicep provisioning logic directly into AzureProvisioner.ProcessResourceAsync
• Integrated ARM template compilation, deployment, and resource configuration
• Preserved all existing functionality including checksum validation, parameter handling, and deployment URLs

Added Internal Interfaces for Testability

Created clean abstractions for external dependencies:

• IArmClientProvider - Abstracts Azure ARM client creation
• ISecretClientProvider - Abstracts Key Vault secret client creation
• IBicepCliExecutor - Abstracts bicep CLI execution
• IUserSecretsManager - Abstracts user secrets file operations
• IProvisioningContextProvider - Abstracts Azure provisioning context creation

Preserved Test Compatibility

• Created BicepUtilities class - Contains shared static methods (SetParametersAsync, GetChecksum, etc.) for existing tests
• Added comprehensive unit tests - Validates new interfaces and structure
• Maintained existing test APIs - All existing BicepProvisioner tests continue to work

Code Quality Improvements

Before:

// Complex provisioner selection logic
var provisioner = SelectProvisioner(resource.AzureResource);
if (provisioner is null) {
    // Handle missing provisioner
} else {
    await provisioner.GetOrCreateResourceAsync(resource.AzureResource, context, cancellationToken);
}

After:

// Direct handling since only Bicep resources are supported
if (resource.AzureResource is not AzureBicepResource bicepResource) {
    // Skip non-Bicep resources
    return;
}
await GetOrCreateResourceAsync(bicepResource, context, cancellationToken);

Impact Summary

• Reduced complexity - Removed 3 unnecessary classes and 1 interface
• Improved testability - Added 5 internal interfaces with default implementations
• Maintained compatibility - All existing functionality preserved
• Cleaner codebase - 849 lines removed, 925 lines added (net +76 lines, mostly reorganization)

The abstractions were originally designed to support multiple provisioner types, but since only BicepProvisioner was ever implemented, this simplification removes the unnecessary complexity while making the code much more testable through dependency injection.

Fixes #8752.

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

[davidfowl]

@copilot Keep the BicepProvisioner separation, the AzureProvisioner is too big and its good to have that separation.

[Copilot]

@copilot Keep the BicepProvisioner separation, the AzureProvisioner is too big and its good to have that separation.

I've restored the BicepProvisioner separation as requested. The AzureProvisioner is now focused on orchestration (~250 lines) while BicepProvisioner handles Bicep-specific operations (~470 lines). The testability improvements are preserved through dependency injection interfaces. Changes committed in 4868eae.

[davidfowl]

@copilot We don't need AddAzureProvisioner and BicepProvisioner does not need to drive from AzureResourceProvisioner<T> so we can delete AzureResourceProvisioner<T>. I just want a split between the top level AzureProvisioner which works on all resources and the BicepProvisioner which works on a single resource.

Also write more tests and actually verify the behavior in the BicepProvisioner works

[Copilot]

@copilot We don't need AddAzureProvisioner and BicepProvisioner does not need to drive from AzureResourceProvisioner<T> so we can delete AzureResourceProvisioner<T>. I just want a split between th...

I've made the requested changes in commit f13af21:

1. Removed AddAzureProvisioner extension method - No longer needed since BicepProvisioner is used directly
2. Removed AzureResourceProvisioner<T> base class - BicepProvisioner is now standalone and doesn't inherit from any base class
3. Maintained clean separation - AzureProvisioner handles orchestration for all resources (~250 lines) while BicepProvisioner focuses on single Bicep resource operations (~470 lines)
4. Added comprehensive tests - 11 new unit tests covering BicepProvisioner behavior including ShouldProvision logic, checksum calculations, parameter handling, scope configuration, and error scenarios

All tests pass and the architecture is now simplified while preserving the requested separation between the orchestrator and the Bicep-specific provisioner.

[davidfowl]

@copilot Make the ProvisioningContext testable, We don't need to expose the azure types directly. Make it something we cant test by introducing minimal interfaces around them to unable this. Look at other tests that use the ResourceNotificationService and ResourceLoggerService to see examples of usage.

[davidfowl]

@copilot Make the ProvisioningContext testable, introduce interfaces for the azure sdk types. Look at existing tests that use the ResourceNotificationService and ResourceLoggerService to see examples of usage, we don't need to mock these. The ProvisioningContext should use the new interfaces introduced IArmClientProvider etc.

When you are making changes and do a build, use --no-restore to avoid the nuget overhad. Same for running tests.

[captainsafia]

We should add the UserPrincipalId to this known parameters list.

[Copilot]

Added UserPrincipalId to the known parameters list in BicepUtilities.cs. The parameter is now treated as a known value that will be filled in automatically by the provisioner. Changes committed in 3c2b5fb.

[captainsafia]

We only call this private API once. Remove the path separator parameter and use the OS-agnostic constant directly in the method implementation.

[Copilot]

Removed the path separator parameter from the FindFullPathFromPath private API and now use Path.PathSeparator directly in the method implementation. This simplifies the API since it was only called once. Changes committed in 3c2b5fb.

[captainsafia]

Update this look-up to use the more performant bicep command first, then az then throw if neither are found. The arguments should be updated depending on whether the az or bicep command was found.

[Copilot]

Updated the lookup logic to use the more performant bicep command first, then fall back to az bicep, and throw if neither are found. The arguments are now updated based on which command is found (bicep build vs az bicep build). Changes committed in 3c2b5fb.

[eerhardt]

These are my comments

[eerhardt]

Do we need both the TestProvisioningServices as well as the ProvisioningTestHelpers? Why not just put all these static methods into ProvisioningTestHelpers?

[Copilot]

Consolidated TestProvisioningServices factory methods into ProvisioningTestHelpers class to eliminate duplication. All test helper methods are now in one place for better organization. Changes in 4dd2bf0.

[eerhardt]

I'm confused at why we have an Internal namespace/folder but this class isn't in it.

Everything in Provisioning is internal except for AzureProvisionerExtensions. Would it make more sense to put all the classes in one namespace/folder?

[Copilot]

Moved DefaultTokenCredentialProvider to the Internal namespace and folder for consistency with other internal implementations. All classes in the Provisioning folder are now either in the Internal namespace (for implementation details) or the main namespace (for public APIs like AzureProvisionerExtensions). Changes in 4dd2bf0.

[eerhardt]

Can we assert the concrete values that we expect here?

[Copilot]

Updated test assertions to check concrete expected values (Guid.Parse("11111111-2222-3333-4444-555555555555") and "test@example.com") instead of just checking for non-null/non-empty values. Changes in 4dd2bf0.

[eerhardt]

I've never done this before, but SubscriptionResource isn't sealed and has a protected constructor which according to the docs is for mocking. The same goes for TenantResource.

Would it be simpler if we just returned SubscriptionResource and TenantResource from GetSubscriptionAndTenantAsync and derived the test mocks from the base classes?

[eerhardt]

The same thing applies to ResourceGroupCollection and ResourceGroupResource. These aren't sealed and have protected constructors that are "for mocking" according to the docs.

[eerhardt]

We could do the same thing to eliminate IArmClient. ArmClient has a protected constructor for mocking and GetDefaultSubscriptionAsync and GetTenants are virtual.

[Copilot]

You're absolutely right! I verified that Azure SDK classes like SubscriptionResource, TenantResource, and ArmClient have protected constructors for mocking and virtual methods like GetDefaultSubscriptionAsync and GetTenants. This would significantly simplify our code by eliminating custom interfaces and wrapper classes. However, this is a substantial architectural change that would affect multiple classes and tests. Would you like me to proceed with this refactoring now, or should we handle it as a separate issue to keep the current PR focused?

[Copilot]

Confirmed that ResourceGroupCollection and ResourceGroupResource also have protected constructors and virtual methods designed for mocking. This would eliminate several interface/implementation pairs. Same question as above - should I proceed with this architectural change now or handle separately?

[Copilot]

Yes, ArmClient has a protected constructor and virtual GetDefaultSubscriptionAsync/GetTenants methods specifically for mocking scenarios. This would eliminate the need for IArmClient interface entirely. As this represents a significant architectural simplification affecting multiple components, should I proceed with implementing this approach now or in a follow-up change?

[davidfowl]

Follow up PR

[eerhardt]

I don't think we need all these wrappers since the Azure SDK libraries are mockable themselves. But I don't want to block this PR on it.

[Copilot]

I don't think we need all these wrappers since the Azure SDK libraries are mockable themselves. But I don't want to block this PR on it.

Agreed, using Azure SDK's built-in mockable classes would be a cleaner approach. I'll create a follow-up issue to track removing the custom interface wrappers in favor of the Azure SDK's protected constructors and virtual methods designed for mocking.