Configure export command #4434

msftrubengu · 2024-05-01T00:23:46Z

This PR introduces the configure export command as an exprimental feature. This is mostly a proof of concept and should not be considered a full feature.

Scenario A - Create a configuration to install a winget package.

Use --pacakgeId with the package identifier of an application in the winget source to produce a configuration file that uses the Microsoft.WinGet.DSC/WinGetPackage resource to install the package via winget. Right now, we don't validate if the package id exists and will just copy what the user sets into the settings of the resource.

Scenario B - 'Export' the configuration from the specified resource.

Use both --module and --resource to get the configuration of a resource and add it to the configuration file. Internally, configuration will install the module (if not installed already) and call Get on the resource. We will then try to serialize its property into the configuration file. Current limitation is that if a resource has a required setting (like WinGetPackage required Id) it won't work. If the resource is not found in the gallery a retry will be performed allowing prereleased modules in the case it exists.

Scenario C - Mix of A and B

If --packageId, --module and --resource are used, configure export will produce two resources. The first one is the WinGetPackage for the specified package. The second one is the same as in B, with the difference that it includes a dependency of the previously created WinGetPackage resource.

Scenario D - Configuration file already exists.

If the file passed to the --output parameters already exists and is a valid configuration file, the resources will be appended. There is currently not validation into the correctness of this, so it can result in a configuration with resources with the same id.

For example winget configure export --packageId Microsoft.AppInstaller --module Microsoft.WinGet.DSC --resource WinGetUserSettings -o test_export.yml would produce the following file

# Created using winget configure export 1.8.0-preview
# yaml-language-server: $schema=https://aka.ms/configuration-dsc-schema/0.2
properties:
  configurationVersion: 0.2
  resources:
  - resource: Microsoft.WinGet.DSC/WinGetPackage
    id: Microsoft.AppInstaller
    directives:
      description: Install Microsoft.AppInstaller
      allowPrerelease: true
    settings:
      id: Microsoft.AppInstaller
      source: winget
  - resource: Microsoft.WinGet.DSC/WinGetUserSettings
    dependsOn:
    - Microsoft.AppInstaller
    directives:
      description: Configure Microsoft.AppInstaller
    settings:
      Settings:
        experimentalFeatures:
          configureSelfElevate: true
        installBehavior:
          preferences:
            locale:
            - en-US
            - fr-FR
        $schema: https://aka.ms/winget-settings.schema.json
      Action: Full

Microsoft Reviewers: Open in CodeFlow

Trenly · 2024-05-01T00:47:02Z

src/AppInstallerCLICore/ConfigureExportCommand.h

+
+namespace AppInstaller::CLI
+{
+    struct ConfigureExportCommand final : public Command


Just curious if there is a design preference here. The subcommands for other areas like pinning are contained within the parent file. For example, PinRemoveCommand is contained in PinCommand.h and SettingsExportCommand is contained in SettingsCommand.h. Is there a reason that all the configuration sub commands wouldn't be included in ConfigureCommand.h ?

My 2🪙: This isn't large enough to warrant a new file, which is to say that if a subcommand were itself rather complex I think it would be ok to break it out (or if it had its own subcommands, etc.)

src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp

Trenly · 2024-05-01T00:54:35Z

src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp

+                winrt::hstring description;
+                if (dependantUnit.has_value())
+                {
+                    description = L"Configure " + dependantUnit.value().Identifier();


I believe it would be best if the descriptions were localized, since it's not a field which performs an action. A user exporting a configuration on a machine would likely prefer their descriptions to be in their preferred locale in the event they need to manually modify it

Trenly · 2024-05-01T01:00:26Z

src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp

    }

    void CreateConfigurationProcessor(Context& context)
    {
        auto progressScope = context.Reporter.BeginAsyncProgress(true);
        progressScope->Callback().SetProgressMessage(Resource::String::ConfigurationInitializing());

-        ConfigurationProcessor processor{ CreateConfigurationSetProcessorFactory(context)};
+        ConfigurationProcessor processor{ anon::CreateConfigurationSetProcessorFactory(context)};


Is there a reason for explicitly naming the anonymous namespace? I haven't seen this done before

I've learned that anonymous namespace does not output type information into the PDB in a way that debuggers (at least VS and WinDBG) can find currently, making it much harder to debug. With a named namespace, the derived type information is found and so the debugger can show the member fields of a derived type (vtable -> type mapping). One can manually do all of this with enough effort, and maybe eventually the debuggers will support it directly, but at this time I see simply naming the namespace as the easiest answer.

src/AppInstallerCommonCore/ExperimentalFeature.cpp

JohnMcPMS · 2024-05-01T16:29:04Z

src/AppInstallerCLICore/ConfigureExportCommand.h

+
+namespace AppInstaller::CLI
+{
+    struct ConfigureExportCommand final : public Command


My 2🪙: This isn't large enough to warrant a new file, which is to say that if a subcommand were itself rather complex I think it would be ok to break it out (or if it had its own subcommands, etc.)

JohnMcPMS · 2024-05-01T16:37:25Z

src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp

+
+                ValueSet settings;
+                settings.Insert(s_Setting_Id, PropertyValue::CreateString(packageIdWide));
+                settings.Insert(s_Setting_Source, PropertyValue::CreateString(s_WinGetSource));


I feel like you either have to require the source to be provided or you need to search for it so you can know the source.

If you searched for it, you could also set the description to Install <NAME> rather than the identifier again.

I don't disagree, but that's a feature request or at least another PR.

src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp

src/Microsoft.Management.Configuration.Processor/Extensions/HashtableExtensions.cs

JohnMcPMS · 2024-05-02T01:22:06Z

src/AppInstallerCLICore/ExecutionArgs.h

@@ -162,7 +162,11 @@ namespace AppInstaller::CLI::Execution
            Max
        };

-        bool Contains(Type arg) const { return (m_parsedArgs.count(arg) != 0); }
+        template<typename... T, std::enable_if_t<(... && std::is_same_v<T, Args::Type>), bool> = true>


Pretty sure you can just do:

Suggested change

template<typename... T, std::enable_if_t<(... && std::is_same_v<T, Args::Type>), bool> = true>

template<Args::Type... T>

JohnMcPMS · 2024-05-02T01:25:15Z

src/AppInstallerCLICore/ExecutionArgs.h

@@ -162,7 +162,11 @@ namespace AppInstaller::CLI::Execution
            Max
        };

-        bool Contains(Type arg) const { return (m_parsedArgs.count(arg) != 0); }
+        template<typename... T, std::enable_if_t<(... && std::is_same_v<T, Args::Type>), bool> = true>
+        bool Contains(T... arg) const


You could also use a std::initializer_list with the low cost of putting {} around your parameter list. But it definitely wouldn't be as fun as figuring out this.

src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp

msftrubengu added 12 commits April 16, 2024 17:18

FF

85d59dc

start cmd

98eecca

Args val

7e39d57

Merge remote-tracking branch 'upstream/master' into config_export

dc8a7b2

anon

60bc7f4

basic command is basic

005683c

Ooook, kind of but needs tests

a62662d

Treat as array

0598518

Merge remote-tracking branch 'upstream/master' into config_export

bd7b98c

Improve

052c1d9

Strings

763f186

Fix and tests

4486fed

msftrubengu requested a review from a team as a code owner May 1, 2024 00:23

This comment has been minimized.

Sign in to view

Trenly reviewed May 1, 2024

View reviewed changes

JohnMcPMS reviewed May 1, 2024

View reviewed changes

msftrubengu added 3 commits May 1, 2024 17:00

PR Comments

62c92eb

schema

ddc162f

Merge remote-tracking branch 'upstream/master' into config_export

4d6a9ad

JohnMcPMS reviewed May 2, 2024

View reviewed changes

bad merge and string

9261581

JohnMcPMS approved these changes May 2, 2024

View reviewed changes

msftrubengu merged commit 5a1631f into microsoft:master May 2, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configure export command #4434

Configure export command #4434

msftrubengu commented May 1, 2024 •

edited by microsoft-github-policy-service bot

This comment has been minimized.

Trenly May 1, 2024

JohnMcPMS May 1, 2024

Trenly May 1, 2024

Trenly May 1, 2024

JohnMcPMS May 1, 2024

JohnMcPMS May 1, 2024

JohnMcPMS May 1, 2024

msftrubengu May 2, 2024

JohnMcPMS May 2, 2024

JohnMcPMS May 2, 2024

	template<typename... T, std::enable_if_t<(... && std::is_same_v<T, Args::Type>), bool> = true>
	template<Args::Type... T>

Configure export command #4434

Configure export command #4434

Conversation

msftrubengu commented May 1, 2024 • edited by microsoft-github-policy-service bot

Scenario A - Create a configuration to install a winget package.

Scenario B - 'Export' the configuration from the specified resource.

Scenario C - Mix of A and B

Scenario D - Configuration file already exists.

Microsoft Reviewers: Open in CodeFlow

This comment has been minimized.

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

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

msftrubengu commented May 1, 2024 •

edited by microsoft-github-policy-service bot