Support fo filesystem-based mocks of registry aliases#19452
Support fo filesystem-based mocks of registry aliases#19452SimonWahlin wants to merge 24 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for filesystem-backed br/<alias>: module aliases so developers can redirect OCI module references to local .bicep files during iteration (Bicep.Core registry/config + tests/baselines).
Changes:
- Extend
moduleAliases.br.<alias>to support afileSystemtarget (mutually exclusive withregistry). - Add an emulated OCI artifact reference type and a registry implementation to load modules directly from disk without restore/cache.
- Update diagnostics/tests and refresh the
Registry_LFbaseline to include an emulated registry alias scenario.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Bicep.Core/Registry/OciArtifactRegistry.cs | Routes br/<alias>: parsing to a filesystem-emulated reference when the alias has fileSystem. |
| src/Bicep.Core/Registry/Oci/OciArtifactReferenceFacts.cs | Adds internal scheme constant for emulated OCI references. |
| src/Bicep.Core/Registry/Oci/OciArtifactEmulatedReference.cs | New artifact reference type resolving br/<alias>: modules to local .bicep files. |
| src/Bicep.Core/Registry/FileSystemModuleRegistry.cs | New registry for br-fs references used during restore/check operations. |
| src/Bicep.Core/Registry/DefaultArtifactRegistryProvider.cs | Registers the new registry and wires IFileExplorer into OciArtifactRegistry. |
| src/Bicep.Core/Modules/ModuleReferenceSchemes.cs | Exposes the internal emulated scheme. |
| src/Bicep.Core/Diagnostics/DiagnosticBuilder.cs | Updates alias diagnostics and hides the internal scheme from “unknown scheme” messages. |
| src/Bicep.Core/Configuration/ModuleAliasesConfiguration.cs | Adds fileSystem to OCI alias config and validates mutual exclusivity. |
| src/Bicep.Core.UnitTests/Utils/OciRegistryHelper.cs | Updates test helper for new OciArtifactRegistry constructor signature. |
| src/Bicep.Core.UnitTests/Registry/OciArtifactEmulatedReferenceTests.cs | New unit tests for parsing/emulated reference behavior and alias validation. |
| src/Bicep.Core.UnitTests/Modules/OciArtifactModuleReferenceTests.cs | Updates expected diagnostics for alias validation changes. |
| src/Bicep.Core.Samples/Files/baselines/Registry_LF/* | Baseline updates to cover filesystem-based alias usage. |
Comments suppressed due to low confidence (1)
src/Bicep.Core/Configuration/ModuleAliasesConfiguration.cs:118
TryGetOciArtifactModuleAliasenforcesregistryXORfileSystem, but it allowsmodulePathto be set alongsidefileSystem. In the current implementation,modulePathis ignored for filesystem-based aliases, which makes that configuration misleading. Consider either (a) rejectingfileSystem+modulePathwith a diagnostic, or (b) applyingmodulePathas an additional subdirectory when resolvingmodulePath.bicepso behavior stays aligned with howmodulePathworks for registry aliases.
if (alias.Registry is not null && alias.FileSystem is not null)
{
return new(x => x.InvalidOciArtifactModuleAliasRegistryAndFileSystemSetTogether(aliasName, configFileUri));
}
if (alias.Registry is null && alias.FileSystem is null)
{
return new(x => x.InvalidOciArtifactModuleAliasRegistryNullOrUndefined(aliasName, configFileUri));
}
return new(alias);
…d diagnostics reserving br-fs scheme for internal use only
…t alias paths relative to bicepconfig
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
|
Looking at this bicepconfig example, it's using the To me it's confusing that the following would map to the file system: module myModule 'br/MyRegistry:mymodule:1.0.0' = {}Whereas the following is clearer: module myModule 'local/myAlias:mymodule' = {} |
|
The purpose of this PR is to be able to locally emulate an ACR to shorten the dev-cycle of modules. Imagine I have a prod ACR where all company modules are hosted. They are of course immutable and won't be changed once published. I have a module (mymodule@1.0.0) that I need to update and release in version 1.1.0. To make sure of the quality of my new module version, I want to deploy a few of my existing templates using this new version of my module. What I would do today is create a personal ACR (or a personal namespace/path in a dev-ACR without immutability), publish my module there for QA and change the module alias to point to my personal ACR. Everyone involved in QA of my module would either have to have access to my personal ACR or publish the module to their own personal ACR. This quickly becomes complicated if the module I'm testing also depends on other modules in the same registry, then they have to be published to my personal ACR as well. With this feature, anyone can check out my PR to the modules repo, point/redirect the module alias to the local repository folder and deploy any template without modification and without worrying about which modules that has to be available as well. That's why I've chosen to call the class OciArtifactEmulatedReference since the whole purpose is to be able to emulate an ACR registry. The EmulatedReference also uses the same br prefix to be able to deploy existing templates without modifications. I don't mind implementing a local or fs alias as well, I think that might be useful instead of using paths like |
I asked the same question earlier, before @SimonWahlin sent the PR. I think he made a good point that requiring the author to change the module path scheme in order to test a new module version would not be ideal. That said, after reading through the samples and test code, I do feel that overloading the existing moduleAliases section could lead to confusion. I have an alternative idea: could we introduce a moduleAliasesMock section that, if present, would supersede moduleAliases? For example: {
"moduleAliasesMock": {
"br": {
"MyRegistry": {
"fileSystem": "bicepModules"
}
}
}
}@SimonWahlin @anthony-c-martin What do you think? |
|
I like the idea of calling it a moduleAliasesMock, that would clear up risk of confusion and also be very clear about the purpose of the feature. |
|
@SimonWahlin Would you like to implement the updated design to use |
|
I would, I have a somewhat working version but I managed to break code completion :). Should have something to show later this week. |
|
@shenglol I've pushed an update, let me know what you think. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 37 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (4)
src/Bicep.Core/Registry/Oci/OciArtifactReferenceFacts.cs:1
- The mocked scheme constant (
"br-mock") is inconsistent with other newly added code/tests that refer to"br-fs"(e.g., diagnostics message andOciArtifactMockedReferenceTestsassertingreference.Scheme == "br-fs"). This will cause test failures and/or confusing behavior; pick one internal scheme string and make it consistent acrossArtifactReferenceSchemes, diagnostics text, and tests.
src/Bicep.IO/Abstraction/IOUri.cs:1 FromFilePathstill duplicates the absolute-path logic thatIsAbsoluteFilePathwas added for. ReuseIsAbsoluteFilePath(filePath)here to avoid future drift between the two checks.
src/Bicep.Core/Registry/OciArtifactMockRegistry.cs:1- The file name (
OciArtifactMockRegistry.cs) doesn’t match the class name (OciArtifactMockedRegistry). Renaming one to match the other will improve discoverability and keep naming consistent with other registry types.
src/vscode-bicep/schemas/bicepconfig.schema.json:1 - The schema under
properties.br.additionalPropertiesincludes both a$refand a siblingadditionalProperties. Many JSON Schema tooling/implementations treat$refas replace/ignore-siblings, so the nestedadditionalProperties: falsemay be ignored and is also redundant because the referenced definition already setsadditionalProperties: false. Remove the sibling keyword (or move constraints into the referenced definition) for clearer and more reliable schema behavior across tooling.
| public override string ToString() => $"{Subscription}/{ResourceGroup}"; | ||
| } | ||
|
|
||
| public record OciArtifactModuleAlias |
There was a problem hiding this comment.
We should avoid adding new properties to the existing OciArtifactModuleAlias and ModuleAliasesConfiguration types solely to support mocking. Even if we exclude FileSystem from the JSON schema, users could still specify a FileSystem property on a regular module alias, and it would still be processed.
A better approach might be to introduce dedicated types such as OciArtifactModuleAliasMock and ModuleAliasesMockConfiguration for mocking scenarios.
Regarding the FileSystem property name, I wonder if MapToFilePath would be clearer, as it more explicitly describes the property's purpose:
{
"moduleAliases": {
"br": {
"myAlias": {
"registry": "example.io",
"modulePath": "modules"
}
}
},
"moduleAliasesMock": {
"br": {
"myAlias": {
// br/myAlias:example.io/modules -> ./foo
"mapToFilePath": "./foo"
}
}
}
}There was a problem hiding this comment.
Great feedback! Thank you.
I'll get right on it :)
There was a problem hiding this comment.
I've pushed an update that hopefully solves this. The checks aren't passing due to identified vulnerability in MessagePack, waiting for #19839 which should fix that.
…and rename Filesystem property to MapToFilePath
I had mock resolution logic here before, now they are just wrapping other methods and are redundant.
Description
When developing modules that are published to a module registry, this feature will add the ability to test existing templates by adding a moduleAliasMock that will redirect a br/alias to a filesystem path.
Fixes #17096
Example Usage
Having an existing template that deploys a module using the following code:
main.bicep
I want to make changes to mymodule and need to validate them before publishing version 1.0.1 to my registry. I update mymodule and save it as
mymodule.bicepin a subfolder namedbicepmodules.bicepmodules/mymodule.bicep
To make my template (main.bicep) use the updated module on my filesystem instead of the one from my container registry, I update my
bicepconfig.jsonto the following:bicepconfig.json
{ "moduleAliases": { "br": { "myregistry": { "registry": "templates.azurecr.io" } } }, "moduleAliasesMock": { "br": { "myregistry": { "fileSystem": "bicepmodules" } } } }I have intentionally not exposed a new prefix for moduleAliases to be able to deploy existing templates that are using the br/ prefix.
Checklist
Microsoft Reviewers: Open in CodeFlow