Skip to content

Commit

Permalink
Sort public registry module version completion items (#15400)
Browse files Browse the repository at this point in the history
The current sortText is not based on semantic versioning and lacks
zero-padding, leading to incorrect client-side module version ordering.
This PR resolves those issues.

Before:


![image](https://github.com/user-attachments/assets/1749d7eb-af40-4884-978d-42e1e07894b9)

After:


![image](https://github.com/user-attachments/assets/9a5acf00-d680-4039-803c-e4ea6512ecb9)


Closes #14530.
###### Microsoft Reviewers: [Open in
CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/Azure/bicep/pull/15400)
  • Loading branch information
shenglol authored Oct 25, 2024
1 parent 9f0de26 commit 03defaa
Show file tree
Hide file tree
Showing 17 changed files with 187 additions and 189 deletions.
2 changes: 1 addition & 1 deletion src/Bicep.Cli.IntegrationTests/LintCommandTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ public async Task Lint_WithEmptyBicepConfig_ShouldProduceConfigurationError()
string testOutputPath = FileHelper.GetUniqueTestOutputPath(TestContext);
var inputFile = FileHelper.SaveResultFile(TestContext, "main.bicep", DataSets.Empty.Bicep, testOutputPath);
var configurationPath = FileHelper.SaveResultFile(TestContext, "bicepconfig.json", string.Empty, testOutputPath);
var settings = new InvocationSettings() { ModuleMetadataClient = PublicRegistryModuleMetadataClientMock.CreateToThrow(new Exception("unit test failed: shouldn't call this")).Object };
var settings = new InvocationSettings() { ModuleMetadataClient = PublicRegistryModuleIndexClientMock.CreateToThrow(new Exception("unit test failed: shouldn't call this")).Object };

var (output, error, result) = await Bicep(settings, "lint", inputFile);

Expand Down
10 changes: 5 additions & 5 deletions src/Bicep.Cli.IntegrationTests/TestBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ namespace Bicep.Cli.IntegrationTests
{
public abstract class TestBase : Bicep.Core.UnitTests.TestBase
{
private static BicepCompiler CreateCompiler(IContainerRegistryClientFactory clientFactory, ITemplateSpecRepositoryFactory templateSpecRepositoryFactory, IPublicRegistryModuleMetadataClient? moduleMetadataClient)
private static BicepCompiler CreateCompiler(IContainerRegistryClientFactory clientFactory, ITemplateSpecRepositoryFactory templateSpecRepositoryFactory, IPublicRegistryModuleIndexClient? moduleMetadataClient)
=> ServiceBuilder.Create(
services =>
{
Expand All @@ -49,21 +49,21 @@ protected record InvocationSettings
public IContainerRegistryClientFactory ClientFactory { get; init; }
public ITemplateSpecRepositoryFactory TemplateSpecRepositoryFactory { get; init; }
public IEnvironment? Environment { get; init; }
public IPublicRegistryModuleMetadataClient ModuleMetadataClient { get; init; }
public IPublicRegistryModuleIndexClient ModuleMetadataClient { get; init; }

public InvocationSettings(
FeatureProviderOverrides? FeatureOverrides = null,
IContainerRegistryClientFactory? ClientFactory = null,
ITemplateSpecRepositoryFactory? TemplateSpecRepositoryFactory = null,
IEnvironment? Environment = null,
IPublicRegistryModuleMetadataClient? ModuleMetadataClient = null)
IPublicRegistryModuleIndexClient? ModuleMetadataClient = null)
{
this.FeatureOverrides = FeatureOverrides;
this.ClientFactory = ClientFactory ?? Repository.Create<IContainerRegistryClientFactory>().Object;
this.TemplateSpecRepositoryFactory = TemplateSpecRepositoryFactory ?? Repository.Create<ITemplateSpecRepositoryFactory>().Object;
this.Environment = Environment;

this.ModuleMetadataClient = ModuleMetadataClient ?? StrictMock.Of<IPublicRegistryModuleMetadataClient>().Object;
this.ModuleMetadataClient = ModuleMetadataClient ?? StrictMock.Of<IPublicRegistryModuleIndexClient>().Object;
}
}

Expand Down Expand Up @@ -109,7 +109,7 @@ protected static void AssertNoErrors(string error)
}
}

protected static async Task<IEnumerable<string>> GetAllDiagnostics(string bicepFilePath, IContainerRegistryClientFactory clientFactory, ITemplateSpecRepositoryFactory templateSpecRepositoryFactory, IPublicRegistryModuleMetadataClient? moduleMetadataClient = null)
protected static async Task<IEnumerable<string>> GetAllDiagnostics(string bicepFilePath, IContainerRegistryClientFactory clientFactory, ITemplateSpecRepositoryFactory templateSpecRepositoryFactory, IPublicRegistryModuleIndexClient? moduleMetadataClient = null)
{
var compilation = await CreateCompiler(clientFactory, templateSpecRepositoryFactory, moduleMetadataClient).CreateCompilation(PathHelper.FilePathToFileUrl(bicepFilePath));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,6 @@ namespace Bicep.Cli.IntegrationTests;
[TestClass]
public class UseRecentModuleVersionsIntegrationTests : TestBase
{
public static BicepModuleMetadata[] DefaultModulesMetadata = [
new BicepModuleMetadata(
"fake/avm/res/app/container-app",
["0.2.0"],
ImmutableDictionary<string, BicepModuleTagPropertiesEntry>.Empty),
];

private const string PREFIX = "br:mcr.microsoft.com/bicep";
private const string NotRestoredErrorCode = "BCP190";
private const string UnableToRestoreErrorCode = "BCP192";
Expand All @@ -47,7 +40,7 @@ public class UseRecentModuleVersionsIntegrationTests : TestBase

private class Options(string CacheRoot)
{
private IPublicRegistryModuleMetadataClient? _metadataClient = null;
private IPublicRegistryModuleIndexClient? _metadataClient = null;
private string? _config = null;

public string Bicep { get; init; } = "/* bicep contents */";
Expand Down Expand Up @@ -81,17 +74,17 @@ public string? BicepConfig
}

// Automatically created from ModulesMetadata by default
public IPublicRegistryModuleMetadataClient MetadataClient
public IPublicRegistryModuleIndexClient MetadataClient
{
set
{
_metadataClient = value;
}
get => _metadataClient is { } ? _metadataClient : PublicRegistryModuleMetadataClientMock.Create(
ModulesMetadata.Select(mm => new BicepModuleMetadata(
get => _metadataClient is { } ? _metadataClient : PublicRegistryModuleIndexClientMock.Create(
ModulesMetadata.Select(mm => new PublicRegistryModuleIndexEntry(
mm.module,
new List<string>(mm.versions),
new Dictionary<string, BicepModuleTagPropertiesEntry>().ToImmutableDictionary()))).Object;
[.. mm.versions],
new Dictionary<string, PublicRegistryModuleProperties>().ToImmutableDictionary()))).Object;
}

}
Expand Down Expand Up @@ -130,7 +123,7 @@ public async Task IfLevelIsOff_ShouldNotDownloadModuleMetadata()
""".Replace("{PREFIX}", PREFIX),
DiagnosticLevel = "off",
PublishedModules = [$"{PREFIX}/fake/avm/res/app/container-app:0.2.0"],
MetadataClient = PublicRegistryModuleMetadataClientMock.CreateToThrow(new Exception("unit test failed: shouldn't try to download in this scenario")).Object,
MetadataClient = PublicRegistryModuleIndexClientMock.CreateToThrow(new Exception("unit test failed: shouldn't try to download in this scenario")).Object,
});

result.Should().NotHaveStderr();
Expand Down Expand Up @@ -172,7 +165,7 @@ public async Task IfMetadataDownloadFails_ThenShouldFail()
}
""".Replace("{PREFIX}", PREFIX),
PublishedModules = [$"{PREFIX}/fake/avm/res/app/container-app:0.2.0"],
MetadataClient = PublicRegistryModuleMetadataClientMock.CreateToThrow(new Exception("Download failed.")).Object,
MetadataClient = PublicRegistryModuleIndexClientMock.CreateToThrow(new Exception("Download failed.")).Object,
});

result.Should().HaveStderrMatch($"*Warning use-recent-module-versions: Could not download available module versions: Download failed.*");
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using System.Collections.Immutable;
using System.Reflection;
using System.Text.RegularExpressions;
using Bicep.Core.Analyzers.Interfaces;
Expand Down Expand Up @@ -43,13 +44,13 @@ private static CompilationResult Compile(
string? downloadError = null)
{
var publicRegistryModuleMetadataProvider = StrictMock.Of<IPublicRegistryModuleMetadataProvider>();
publicRegistryModuleMetadataProvider.Setup(x => x.GetCachedModules())
.Returns(availableModules.Select(m => new RegistryModule(m, null, null)).ToArray());
publicRegistryModuleMetadataProvider.Setup(x => x.GetCachedModuleVersions(It.IsAny<string>()))
publicRegistryModuleMetadataProvider.Setup(x => x.GetModulesMetadata())
.Returns(availableModules.Select(m => new PublicRegistryModuleMetadata(m, null, null)).ToImmutableArray());
publicRegistryModuleMetadataProvider.Setup(x => x.GetModuleVersionsMetadata(It.IsAny<string>()))
.Returns((string module) =>
{
return availableModules.Contains(module) ?
availableVersions.Select(v => new RegistryModuleVersion(v, null, null)).ToArray() :
availableVersions.Select(v => new PublicRegistryModuleVersionMetadata(v, null, null)).ToImmutableArray() :
[];
});
publicRegistryModuleMetadataProvider.Setup(x => x.IsCached)
Expand Down
2 changes: 1 addition & 1 deletion src/Bicep.Core.UnitTests/IServiceCollectionExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public static IServiceCollection AddBicepCore(this IServiceCollection services)
.AddPublicRegistryModuleMetadataProviderServices()
.AddSingleton<BicepCompiler>();

AddMockHttpClient(services, PublicRegistryModuleMetadataClientMock.Create([]).Object);
AddMockHttpClient(services, PublicRegistryModuleIndexClientMock.Create([]).Object);

return services;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using System;
using System.Collections.Immutable;
using Bicep.Core.Registry.PublicRegistry;
using Bicep.Core.UnitTests.Mock;
using Moq;

namespace Bicep.Core.UnitTests.Mock;

public static class PublicRegistryModuleIndexClientMock
{
// CONSIDER: Mock HttpClient rather than the typed client

public static Mock<IPublicRegistryModuleIndexClient> Create(IEnumerable<PublicRegistryModuleIndexEntry> metadata)
{
var mock = StrictMock.Of<IPublicRegistryModuleIndexClient>();
mock
.Setup(client => client.GetModuleIndexAsync())
.ReturnsAsync(() => metadata.ToImmutableArray());
return mock;
}

public static Mock<IPublicRegistryModuleIndexClient> CreateToThrow(Exception exception)
{
var mock = StrictMock.Of<IPublicRegistryModuleIndexClient>();
mock
.Setup(client => client.GetModuleIndexAsync())
.ThrowsAsync(exception);
return mock;
}
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ private IServiceProvider GetServiceProvider()
{
var httpClient = MockHttpMessageHandler.ToHttpClient();
return new ServiceBuilder().WithRegistration(x =>
x.AddSingleton<IPublicRegistryModuleMetadataClient>(new PublicRegistryModuleMetadataClient(httpClient))
x.AddSingleton<IPublicRegistryModuleIndexClient>(new PublicRegistryModuleMetadataClient(httpClient))
).Build().Construct<IServiceProvider>();
}
private const string ModuleIndexJson = """
Expand Down Expand Up @@ -764,7 +764,9 @@ private IServiceProvider GetServiceProvider()
"moduleName": "samples/array-loop",
"tags": [
"1.0.1",
"1.10.1",
"1.0.2",
"1.0.2-preview",
"1.0.3"
],
"properties": {
Expand Down Expand Up @@ -1174,7 +1176,7 @@ public async Task GetModules_Count_SanityCheck()
{
PublicRegistryModuleMetadataProvider provider = new(GetServiceProvider());
(await provider.TryUpdateCacheAsync()).Should().BeTrue();
var modules = provider.GetCachedModules();
var modules = provider.GetModulesMetadata();
modules.Should().HaveCount(50);
}

Expand All @@ -1183,7 +1185,7 @@ public async Task GetModules_OnlyLastTagHasDescription()
{
PublicRegistryModuleMetadataProvider provider = new(GetServiceProvider());
(await provider.TryUpdateCacheAsync()).Should().BeTrue();
var modules = await provider.GetModules();
var modules = provider.GetModulesMetadata();
var m = modules.Should().Contain(m => m.Name == "samples/hello-world")
.Which;
m.Description.Should().Be("A \"שָׁלוֹם עוֹלָם\" sample Bicep registry module");
Expand All @@ -1195,11 +1197,27 @@ public async Task GetModules_MultipleTagsHaveDescriptions()
{
PublicRegistryModuleMetadataProvider provider = new(GetServiceProvider());
(await provider.TryUpdateCacheAsync()).Should().BeTrue();
var modules = await provider.GetModules();
var modules = provider.GetModulesMetadata();
var m = modules.Should().Contain(m => m.Name == "lz/sub-vending")
.Which;
m.Description.Should().Be("This module is designed to accelerate deployment of landing zones (aka Subscriptions) within an Azure AD Tenant.");
m.DocumentationUri.Should().Be("https://github.com/Azure/bicep-registry-modules/tree/lz/sub-vending/1.4.2/modules/lz/sub-vending/README.md");
}

[TestMethod]
public async Task GetModuleVerionsMetadata_ByDefault_ReturnsMetadataSortedByVersion()
{
PublicRegistryModuleMetadataProvider provider = new(GetServiceProvider());
(await provider.TryUpdateCacheAsync()).Should().BeTrue();

var versions = provider.GetModuleVersionsMetadata("samples/array-loop").Select(x => x.Version);

versions.Should().Equal(
"1.10.1",
"1.0.3",
"1.0.2",
"1.0.2-preview",
"1.0.1");
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ private static IEnumerable<Failure> GetFailures(SemanticModel model, IServicePro

private static IEnumerable<Failure> AnalyzeBicepModule(IPublicRegistryModuleMetadataProvider publicRegistryModuleMetadataProvider, ModuleDeclarationSyntax moduleSyntax, TextSpan errorSpan, string tag, string publicModulePath)
{
var availableVersions = publicRegistryModuleMetadataProvider.GetCachedModuleVersions(publicModulePath)
var availableVersions = publicRegistryModuleMetadataProvider.GetModuleVersionsMetadata(publicModulePath)
.Select(v => v.Version)
.ToArray();
if (availableVersions.Length == 0)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using System.Collections.Immutable;
using System.Text.Json.Serialization;
using Bicep.Core.Extensions;
using Bicep.Core.Registry.Oci;
using Semver;

namespace Bicep.Core.Registry.PublicRegistry;

public readonly record struct PublicRegistryModuleProperties(string Description, string DocumentationUri);

public record PublicRegistryModuleIndexEntry(
[property:JsonPropertyName("moduleName")]string ModulePath, // e.g. "avm/app/dapr-containerapp"
ImmutableArray<string> Tags, // e.g. "1.0.0" (not guaranteed to be in that format, although it currently is for public modules)
[property: JsonPropertyName("properties")] ImmutableDictionary<string, PublicRegistryModuleProperties> PropertiesByTag // Module properties per tag
)
{
private static readonly SemVersion DefaultVersion = new(0);

// Sort tags by version numbers in descending order.
public ImmutableArray<string> Versions { get; } = [.. Tags.OrderByDescending(x => SemVersion.TryParse(x, SemVersionStyles.AllowV, out var version) ? version : DefaultVersion)];

public string? GetDescription(string? version = null) => this.GetProperty(version, x => x.Description);

public string? GetDocumentationUri(string? version = null) => this.GetProperty(version, x => x.DocumentationUri);

private string? GetProperty(string? version, Func<PublicRegistryModuleProperties, string> propertySelector)
{
if (version is null)
{
// Get description for most recent version with a description
foreach (var tag in this.Versions)
{
if (this.PropertiesByTag.TryGetValue(tag, out var propertiesEntry))
{
return propertySelector(propertiesEntry);
}
}

return null;
}
else
{
return this.PropertiesByTag.TryGetValue(version, out var propertiesEntry) ? propertySelector(propertiesEntry) : null;
}
}
}

public interface IPublicRegistryModuleIndexClient
{
Task<ImmutableArray<PublicRegistryModuleIndexEntry>> GetModuleIndexAsync();
}

This file was deleted.

Loading

0 comments on commit 03defaa

Please sign in to comment.