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

Add configuration environment #5139

Merged
merged 19 commits into from
Jan 21, 2025
Merged

Conversation

JohnMcPMS
Copy link
Member

@JohnMcPMS JohnMcPMS commented Jan 15, 2025

Change

This change adds a ConfigurationEnvironment type which is a property of a ConfigurationUnit. It defines the environment in which the unit should be run. This currently encompasses two properties; the security context and the processor.

The values are parsed from the metadata, but the object is considered authoritative at runtime and when serializing. They are also inherited in schema 0.3, allowing the entire set to be defined in a single environment in it's metadata.

The code that was using the securityContext metadata has been updated to use the environment property instead. It also uses the unique environment calculation function for the set to more efficiently determine which contexts are present.

Validation

New tests are added for parsing, serialization, and unique environment calculation.

Microsoft Reviewers: Open in CodeFlow

@JohnMcPMS JohnMcPMS requested a review from a team as a code owner January 15, 2025 19:47

This comment was marked as outdated.

@JohnMcPMS
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JohnMcPMS
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JohnMcPMS
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

yao-msft
yao-msft previously approved these changes Jan 18, 2025
[contract(Microsoft.Management.Configuration.Contract, 3)]
{
// Gets the union of environments as defined by all of the active units within the set.
Windows.Foundation.Collections.IVector<IConfigurationEnvironmentView> GetUnitEnvironments();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be IVectorView if it's read only? Same for line 1013 below.

Copy link
Member Author

Choose a reason for hiding this comment

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

A view would be a read only collection, but I'm giving back a collection that the caller now owns. They can change it all they want, but it contains views that they cannot change.

But this has made me reconsider having IConfigurationEnvironmentView in the first place, which was originally part of a more complex change. I don't think it is needed any more and could even lead to some incorrect assumptions given how things are implemented. I'm going to return a vector of environments that are copies of the ones they represent and remove the environment view completely.


void ComputeUniqueEnvironments(std::vector<EnvironmentData>& uniqueEnvironments, const Windows::Foundation::Collections::IVector<Configuration::ConfigurationUnit>& units)
{
for (Configuration::ConfigurationUnit unit : units)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: const &


// Attempts to parse a security context from a string.
// Returns true if successful; false otherwise.
bool TryParseSecurityContext(hstring value, SecurityContext& result);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Do we need const hstring& for this and below?


void ConfigurationEnvironment::DeepCopy(const implementation::ConfigurationEnvironment& toDeepCopy)
{
m_context = toDeepCopy.m_context;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is duplicate code with line 23. Is it possible to make it a common method that works on IConfigurationEnvironmentView?


std::wstring_view GetSchemaVersionCommentPrefix();

private:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: not needed

@JohnMcPMS
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JohnMcPMS JohnMcPMS merged commit e69201b into microsoft:master Jan 21, 2025
9 checks passed
@JohnMcPMS JohnMcPMS deleted the config-dyn-fact branch January 21, 2025 23:31
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

2 participants