Add cloud and module alias configurations #4576

shenglol · 2021-09-24T06:56:24Z

Changes

Refactored the implementation of bicep configuration and added configuration sections for cloud and module aliases, which is a prerequisite for #4533.

ConfigHelper and IConfigurationRoot were replaced with a cleaner ConfigurationManager class and several strongly typed configuration models.

The PR also fixes #4471 and #4533.

I'll create a separate PR to update the JSON schema for bicepconfig.json.

anthony-c-martin · 2021-09-24T20:23:14Z

src/Bicep.Core/Configuration/ConfigurationManager.cs

+        public RootConfiguration GetBuiltInConfiguration(bool disableAnalyzers = false) => disableAnalyzers
+            ? builtInConfigurationWithAnalyzersDisabledLazy.Value
+            : builtInConfigurationLazy.Value;


I thought we'd pulled out support for setting disableAnalyzers entirely. Do you know which code paths this is being used in?

Yup. I thought about removing this as well but later realized that this is needed to fix #4471. If a custom bicepconfig.json file is invalid we will fallback to the default configuration with linter disabled so the language server can still work (see https://github.com/Azure/bicep/pull/4576/files#diff-3707a23954e587bd8ed9df408b7c7990474a7bf1765d48e00b6930516ad39c28R299-R300).

Wouldn't we just want to fall back to the default default configuration in this scenario?

One justification is that if the user have some default linter rules disabled in their broken bicepconfig.json, they will get warnings for those rules which could be confusing.

anthony-c-martin · 2021-09-24T20:25:36Z

src/Bicep.Core/Configuration/ConfigurationManager.cs

+            }
+            catch (Exception)
+            {
+                return null;


This feels a bit problematic, although surfacing the exception to the user may also end up showing false positives (e.g. if the user account has missing permissions on a parent dir). I think it might be worth creating an issue to try and get telemetry on this. As a user I feel like I'd rather understand if there was an issue loading my config - without this the experience may appear unpredictable.

This was the original code in ConfigHelper, but I think you are right that we should definitely not drop the exception silently and return null. I've updated the logic so that any IO related exception will converted to a ConfigurationException with an error message like Error while discovering Bicep configuration file: \"{exception.message}\"..

src/Bicep.Core/Diagnostics/DiagnosticBuilder.cs

src/Bicep.Core/IsInternalInit.cs

src/Bicep.Cli/Program.cs

src/Bicep.Cli.IntegrationTests/BuildCommandTests.cs

src/Bicep.LangServer/Handlers/BicepBuildCommandHandler.cs

src/Bicep.LangServer/Server.cs

src/Bicep.Core/Configuration/bicepconfig.json

majastrz · 2021-09-28T01:00:58Z

    public string DisableLinterRule(string bicepConfig, string code)

Should we change this logic to operatote on the strongly-typed model?

Refers to: src/Bicep.LangServer/Handlers/BicepDisableLinterRuleCommandHandler.cs:53 in 3c0ae01. [](commit_id = 3c0ae01, deletion_comment = False)

majastrz · 2021-09-28T01:08:09Z

src/Bicep.Core/Configuration/ConfigurationManager.cs

+                    using var stream = fileSystem.FileStream.Create(customConfigurationPath, FileMode.Open, FileAccess.Read);
+                    var builder = new ConfigurationBuilder()
+                        .AddInMemoryCollection(builtInRawConfiguration.AsEnumerable())
+                        .AddJsonStream(stream);


AddJsonStream

I'm assuming this does a merge of the file with the default config.

What are the semantics on merging objects? Do objects get replaced or does it combine the properties? And is it recursive? Do property values get replaced or merged as well?

The semantic is patch on merging objects. It combines properties and it is recursive. Any property value will get replaced if the value type is string, int, bool, or null. For array, the behavior is a bit weird - each array item in the original config will get replaced by the new array item in the new config in order. E.g.:

// old config { "numbers": [1, 2, 3, 4] } // new config { "numbers": [0, 0] } // merged config { "numbers": [0, 0, 3, 4] }

This makes me wonder if we should customize array merging...with the current behavior it's very tricky to override disallowedhosts and excludedhosts.

Yeah I think you're right. Is it possible to customize that in the config library?

Currently it's not possible. It's tracked by dotnet/runtime#36569. I was going to create a a Bicep issue for the task but turned out there already exists one: #4145. We can address this later in a separate PR.

shenglol · 2021-09-28T01:30:59Z

Should we change this logic to operatote on the strongly-typed model?

Yeah...I was expecting this after merging main. I'll make the change.

@majastrz Update: after checking the implementation I found it pretty hard to have the strongly-typed model handle linter rule overriding. The problem is that the strongly-typed model for the linter rules is just a wrapper for IConfiguration, which is basically a <config key, config value string> collection. We can try and create a JSON serializer for it, but we will have to do type conversions to convert a string to a strong type, which worries me. It might work for the Core linter rules since we know the schema, however, it won't scale once we add support for custom linter rules. I feel like we'll need to have a discussion about this.

src/Bicep.Core/Configuration/bicepconfig.json

src/Bicep.Core/Configuration/ConfigurationManager.cs

src/Bicep.Core/Configuration/RootConfiguration.cs

anthony-c-martin

Looks great! Nice job cleaning up the config classes 😄

Add cloud and module alias configurations

33ee940