work

Author: StephenWeatherford

src/Bicep.Cli.IntegrationTests/UseRecentModuleVersionsIntegrationTests.cs

@@ -111,33 +111,11 @@ private async Task<CliResult> Test(Options options)
         return await Bicep(settings, "lint", mainFile, options.NoRestore ? "--no-restore" : null);
     }
 
-    [TestMethod] //asdfgasdfgasdfg2 should be failing exception is swallowed
-    public async Task IfLevelIsOff_ShouldNotDownloadModuleMetadata()
-    {
-        var moduleIndexClientMock = PublicRegistryModuleIndexClientMock.CreateToThrow(new Exception("shouldn't try to download metadata if rule is off"));
-        var result = await Test(new Options(CacheRoot)
-        {
-            Bicep = """
-                module m1 '{PREFIX}/fake/avm/res/app/container-app:0.2.0' = {
-                  name: 'm1'
-                }
-                """.Replace("{PREFIX}", PREFIX),
-            DiagnosticLevel = "off",
-            PublishedModules = [$"{PREFIX}/fake/avm/res/app/container-app:0.2.0"],
-            MetadataClient = moduleIndexClientMock.Object,
-        });
-
-        result.Should().NotHaveStderr();
-        result.Should().HaveStdout("");
-        result.Should().Succeed();
-
-        moduleIndexClientMock.Verify(client => client.GetModuleIndexAsync(), Times.Never, "shouldn't try to download metadata if rule is off");
-    }
-
     [TestMethod]
     // We don't currently cache to disk, but rather on every check to restore modules.
-    public async Task IfNoRestoreSpecified_ThenShouldFailBecauseNoCache()
+    public async Task IfNoRestoreSpecified_ThenShouldNotDownloadMetadata_AndShouldFailBecauseNoCache()
     {
+        var moduleIndexClientMock = PublicRegistryModuleIndexClientMock.CreateToThrow(new Exception("shouldn't try to download metadata --no-restore is set"));
         var result = await Test(new Options(CacheRoot)
         {
             Bicep = """
@@ -147,13 +125,16 @@ public async Task IfNoRestoreSpecified_ThenShouldFailBecauseNoCache()
                 """.Replace("{PREFIX}", PREFIX),
             PublishedModules = [$"{PREFIX}/fake/avm/res/app/container-app:0.2.0"],
             ModulesMetadata = [("fake/avm/res/app/container-app", ["0.2.0"])],
+            MetadataClient = moduleIndexClientMock.Object,
             NoRestore = true,
         });
 
         result.Should().HaveStderrMatch($"*Error {NotRestoredErrorCode}: The artifact with reference \"br:mcr.microsoft.com/bicep/fake/avm/res/app/container-app:0.2.0\" has not been restored.*");
         result.Should().HaveStderrMatch("*Warning use-recent-module-versions: Available module versions have not yet been downloaded. If running from the command line, be sure --no-restore is not specified.*");
         result.Should().HaveStdout("");
         result.Should().Fail();
+
+        moduleIndexClientMock.Verify(client => client.GetModuleIndexAsync(), Times.Never, "shouldn't try to download metadata --no-restore is set");
     }
 
     [TestMethod]

src/Bicep.Core/Analyzers/Linter/Rules/UseRecentModuleVersionsRule.cs

@@ -118,6 +118,9 @@ private static IEnumerable<Failure> GetFailures(SemanticModel model, IServicePro
 
         private static IEnumerable<Failure> AnalyzeBicepModule(IPublicRegistryModuleMetadataProvider publicRegistryModuleMetadataProvider, ModuleDeclarationSyntax moduleSyntax, TextSpan errorSpan, string tag, string publicModulePath)
         {
+            // NOTE: We don't want linter tests to download anything during analysis.  So metadata is loaded
+            //   and cached during module restore.  So don't use the Get*Async methods of IPublicRegistryModuleMetadataProvider,
+            //   just the GetCached* methods
             var availableVersions = publicRegistryModuleMetadataProvider.GetCachedModuleVersions($"{LanguageConstants.BicepPublicMcrPathPrefix}{publicModulePath}")
                 .Select(v => v.Version)
                 .ToArray();

src/Bicep.Core/Registry/OciArtifactRegistry.cs

@@ -35,7 +35,7 @@ public sealed class OciArtifactRegistry : ExternalArtifactRegistry<OciArtifactRe
 
         private readonly IFeatureProvider features;
 
-        private readonly IPublicRegistryModuleMetadataProvider/*asdfg or indexer?*/ publicRegistryModuleMetadataProvider;
+        private readonly IPublicRegistryModuleMetadataProvider publicRegistryModuleMetadataProvider;
 
         public OciArtifactRegistry(
             IFileResolver FileResolver,
@@ -218,10 +218,15 @@ private OciManifest GetCachedManifest(OciArtifactReference ociArtifactModuleRefe
 
         public override async Task OnRestoreArtifacts(bool forceRestore)
         {
-            // We call this here because we don't want to run this code at all if we're running from the command line and --no-restore has been specified.
-            // So, if we're restoring artifacts, it's safe to also query for public module metadata
-            //asdfg use global options instead?
-            await publicRegistryModuleMetadataProvider.TryAwaitCache(forceRestore); //asdfg why are we waiting for the cache?   asdfg should we be using the indexer instead?
+            // We don't want linter tests to download anything during analysis.  So we are downloading
+            //   metadata here to avoid downloading during analysis, and tests can use cached data if it
+            //   exists (e.g. IRegistryModuleMetadataProvider.GetCached* methods).
+            // If --no-restore has been specified on the command ine, we don't want to download anything at all.
+            // Therefore we do the cache download here so that lint rules can have access to the cached metadata.
+            // CONSIDER: Revisit if it's okay to download metadata during analysis?  This will be more of a problem
+            //   when we extend the linter rules to include private registry modules.
+
+            await publicRegistryModuleMetadataProvider.TryAwaitCache(forceRestore);
         }
 
         public override async Task<IDictionary<ArtifactReference, DiagnosticBuilder.DiagnosticBuilderDelegate>> RestoreArtifacts(IEnumerable<OciArtifactReference> references)