Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configuration processors #3008

Merged
merged 22 commits into from Mar 1, 2023

Conversation

msftrubengu
Copy link
Contributor

@msftrubengu msftrubengu commented Feb 23, 2023

This work is also a small part of #2845

Change

Microsoft.Management.Configuration.Process is a cswinrt component that provides implementation of IConfigurationSetProcessor, IConfigurationUnitProcessor, IConfigurationUnitProcessorDetails, IConfigurationUnitSettingDetails and IConfigurationProcessorFactory defined in #2942 as well as new runtime classes and an interface.

ConfigurationSetProcessorFactory implements IConfigurationProcessorFactory, which takes the type of configuration and optional IConfigurationProcessorFactoryProperties with additional properties that need to be defined for the processor set. An implementation of IConfigurationProcessorFactoryProperties is also provided.

Internally, it uses PowerShell Host to find and install DSC modules and apply them via InvokeDscResource.

Requires PSDesiredStateConfiguration 2.0.6

Validation

Added unit tests and test locally full E2E.

Microsoft Reviewers: Open in CodeFlow

@msftrubengu msftrubengu requested a review from a team as a code owner February 23, 2023 22:19
@github-actions

This comment has been minimized.

@ryfu-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@github-actions

This comment has been minimized.

@@ -189,7 +194,8 @@ LPWCH
LPWSTR
LSTATUS
LTDA
maclachlan
luffy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import anime_expect.txt

internal static class Modules
{
public const string PSDesiredStateConfiguration = "PSDesiredStateConfiguration";
public const string PSDesiredStateConfigurationMinVersion = "2.0.6";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we ignoring the PowerShellGet minimum version requirement for now? I think I would rather we fail explicitly in that case than inexplicably, even if we aren't taking any actions to correct it.

}
else if (dscResourceInfos.Count > 1)
{
throw new ArgumentException(name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there isn't a better built in exception type for "not specific enough", then you should probably create one.

Actually, this applies a lot of places below. I don't think ArgumentException is appropriate for anything after calling into a dependency (you should be able to check for ArgumentException from the arguments and object state alone).

// Even if the windows system32 windows powershell module path is removed they
// will show up.
if (dscResourceInfo.ParentPath!.StartsWith(
@"C:\WINDOWS\system32\WindowsPowershell\v1.0\Modules\PsDesiredStateConfiguration\DscResources",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider a readonly array in your constants to contain these exclusions.

/// <summary>
/// Resource not found by Find-DscResource.
/// </summary>
internal class FindDscResourceNotFoundException : Exception
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should add a new error HR to the native code (shared lib) and then set the exception HR for these to that so that we can have a good mapping between the two.

/// Initializes a new instance of the <see cref="ConfigurationProcessorFactoryProperties"/> class.
/// </summary>
/// <param name="additionalModulePaths">Additional module paths.</param>
public ConfigurationProcessorFactoryProperties(IReadOnlyList<string> additionalModulePaths)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a default constructor and all of the properties should be settable.

if (dscResourceInfo is null)
{
// Well, this is awkward.
return null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe an exception instead?

@msftrubengu msftrubengu merged commit e66cbc9 into microsoft:master Mar 1, 2023
@msftrubengu msftrubengu deleted the configuration_processors branch April 7, 2023 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants