Config framework implementation #17545

isra-fel · 2022-03-22T03:54:57Z

This PR implements the abstractions defined in Azure/azure-powershell-common#313

You can review this PR by commits:

8efa0f9 brought in the code of Microsoft.Extensions.Configuration library we use as the storage layer, with some modification that allows us to update the configuration; to read environment variables from different targets (user/system) etc.
e54b3cb is the main implementation of the interfaces, the core types being ConfigManager which provides all the CRUD APIs to the config; and ConfigInitializer which initializes the config file and config manager. This commit also includes helper classes and abstract config definitions.
3f59938 is about the PowerShell cmdlets, for example, Get-AzConfig.
c1e967f is tests.

Description

Checklist

I have read the Submitting Changes section of CONTRIBUTING.md
The title of the PR is clear and informative
The appropriate ChangeLog.md file(s) has been updated:
- For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
- A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
The PR does not introduce breaking changes
If applicable, the changes made in the PR have proper test coverage
For public API changes to cmdlets:
- a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
  - https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/1109
- the markdown help files have been regenerated using the commands listed here

src/Accounts/.editorconfig

src/Accounts/Accounts.sln

src/Accounts/Accounts/Az.Accounts.psd1

src/Accounts/Accounts/Config/ClearConfigCommand.cs

src/Accounts/Accounts/Config/ConfigCommandBase.cs

src/Accounts/Authentication/Config/ConfigExample/RetryConfig.cs

src/Accounts/Authentication/Config/ConfigInitializer.cs

src/Accounts/AuthenticationAssemblyLoadContext/AuthenticationAssemblyLoadContext.csproj

src/Accounts/Authentication/Config/Internal/ConfigurationRoot.cs

isra-fel · 2022-03-24T05:40:23Z

src/Accounts/Accounts/help/Clear-AzConfig.md

+```
+
+## DESCRIPTION
+{{ Fill in the Description }}


Docs will be updated in a following PR.
I merely included them for CI.

isra-fel · 2022-03-24T08:25:11Z

src/Accounts/Authentication/Config/ConfigInitializer.cs

+            }
+        }
+
+        // todo: tests initializes configs in a different way. Maybe there should be an abstraction IConfigInitializer and two concrete classes ConfigInitializer + TestConfigInitializer


This refactoring is low priority

isra-fel · 2022-03-24T08:25:35Z

src/Accounts/Authentication/Config/ConfigInitializer.cs

+            configManager.BuildConfig();
+        }
+
+        private void RegisterConfigs(IConfigManager configManager)


This method will be implemented in a follow up PR

It is great that those code can be removed.

src/Accounts/Accounts/Config/ConfigCommandBase.cs

dingmeng-xue · 2022-04-12T02:29:29Z

src/Accounts/Authentication/Config/Helper/AppliesToHelper.cs

+    public static class AppliesToHelper
+    {
+        internal static readonly Regex ModulePattern = new Regex(@"^az\.[a-z]+$", RegexOptions.IgnoreCase);
+        internal static readonly Regex CmdletPattern = new Regex(@"^[a-z]+-[a-z]+$", RegexOptions.IgnoreCase);


Cmdlet may contains digital number. such as Get-AzDataFactoryV2 I recommend that module supports digital number too.

Added numbers to the regex and moved the logic of handling regex into a new PSNamingUtilities class.

dingmeng-xue · 2022-04-12T02:36:39Z

src/Accounts/Authentication/Authentication.csproj

@@ -30,4 +30,8 @@
    </EmbeddedResource>
  </ItemGroup>

+  <ItemGroup>


Can we remove this part? it seems it is not required.

dingmeng-xue · 2022-04-14T14:38:51Z

src/Accounts/Authentication/Config/Helper/JsonConfigWriter.cs

+        private bool IsMultiContent(object value)
+        {
+            return value is Array;
+        }


seems not required. nice to have.

Agreed, nice to have.

dingmeng-xue · 2022-04-14T15:12:58Z

A general question. If class is defined as interface, why is its namespace internal?

… config

isra-fel · 2022-05-12T02:37:36Z

A general question. If class is defined as interface, why is its namespace internal?

The code in the internal namespace works as the data layer of the config framework. It is not supposed to be exposed as public API, so I made it explicit by the namespace.

isra-fel commented Mar 22, 2022

View reviewed changes

isra-fel commented Mar 23, 2022

View reviewed changes

src/Accounts/AuthenticationAssemblyLoadContext/AuthenticationAssemblyLoadContext.csproj Outdated Show resolved Hide resolved

isra-fel commented Mar 23, 2022

View reviewed changes

src/Accounts/Authentication/Config/Internal/ConfigurationRoot.cs Outdated Show resolved Hide resolved

isra-fel commented Mar 24, 2022

View reviewed changes

isra-fel added 4 commits March 24, 2022 16:38

Modified code from Microsot.Extensions.Configuration

8efa0f9

implementation of config framework

e54b3cb

cmdlet related

3f59938

test related

c1e967f

isra-fel force-pushed the config branch from 8a59c0e to c1e967f Compare March 24, 2022 08:41

isra-fel commented Mar 24, 2022

View reviewed changes

isra-fel marked this pull request as ready for review March 24, 2022 08:53

changelog; 3rd party license

678bef2

isra-fel force-pushed the config branch from 3276086 to 678bef2 Compare March 24, 2022 08:56

move PSConfig model to Accounts proj; feedback link

ceebb4a

isra-fel force-pushed the config branch from 51b8854 to ceebb4a Compare March 25, 2022 05:43

isra-fel added 2 commits March 25, 2022 15:24

Move preview attribute into child classes

1a6a670

fix get/update by config key

4f6a903

dingmeng-xue reviewed Apr 12, 2022

View reviewed changes

isra-fel self-assigned this Apr 12, 2022

isra-fel added the needs-revision label Apr 12, 2022

dingmeng-xue reviewed Apr 14, 2022

View reviewed changes

isra-fel added 2 commits May 9, 2022 11:13

correct cmdlet regex

1283ed6

Merge branch 'main' of https://github.com/Azure/azure-powershell into…

86ef6eb

… config

isra-fel added needs-review and removed needs-revision labels May 12, 2022

isra-fel assigned dingmeng-xue and unassigned isra-fel May 12, 2022

dingmeng-xue approved these changes May 12, 2022

View reviewed changes

dingmeng-xue added Waiting for CI and removed needs-review labels May 12, 2022

isra-fel merged commit 7d4bee9 into Azure:main May 12, 2022

isra-fel mentioned this pull request May 26, 2022

Fix exception import Az.Accounts caused by environment variables #18311

Closed

8 tasks

isra-fel deleted the config branch July 1, 2022 08:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Config framework implementation #17545

Config framework implementation #17545

isra-fel commented Mar 22, 2022 •

edited

Loading

isra-fel Mar 24, 2022

isra-fel Mar 24, 2022

isra-fel Mar 24, 2022

dingmeng-xue Apr 12, 2022

isra-fel May 9, 2022

dingmeng-xue Apr 12, 2022 •

edited

Loading

isra-fel May 9, 2022

dingmeng-xue Apr 12, 2022

isra-fel May 9, 2022

dingmeng-xue Apr 14, 2022

isra-fel May 9, 2022

dingmeng-xue commented Apr 14, 2022

isra-fel commented May 12, 2022

Config framework implementation #17545

Config framework implementation #17545

Conversation

isra-fel commented Mar 22, 2022 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dingmeng-xue Apr 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dingmeng-xue commented Apr 14, 2022

isra-fel commented May 12, 2022

isra-fel commented Mar 22, 2022 •

edited

Loading

dingmeng-xue Apr 12, 2022 •

edited

Loading