-
Notifications
You must be signed in to change notification settings - Fork 202
Add support for importantSettings to override lower priority settings #2293
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
base: main
Are you sure you want to change the base?
Changes from all commits
9e11aa9
dfcf393
a9f9458
2601513
428e1d8
f4b42e6
9872b5e
72da040
16a19a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,13 +16,19 @@ $CustomTemplateProjectSettingsFile = Join-Path '.github' $CustomTemplateProjectS | |
| function MergeCustomObjectIntoOrderedDictionary { | ||
| Param( | ||
| [System.Collections.Specialized.OrderedDictionary] $dst, | ||
| [PSCustomObject] $src | ||
| [PSCustomObject] $src, | ||
| [string[]] $srcImportantSettings = @(), | ||
| [string[]] $dstImportantSettings = @() | ||
| ) | ||
|
|
||
| # If the src object contains property 'overwriteSettings' (list of settings), remove these settings from the dst object, so that they can be re-added with the new value later on | ||
| if ($src.PSObject.Properties.Name -contains "overwriteSettings") { | ||
| $src.overwriteSettings | ForEach-Object { | ||
| $prop = $_ | ||
| if ($dstImportantSettings -contains $prop -and $srcImportantSettings -notcontains $prop) { | ||
| OutputDebug "Ignoring overwriteSettings for '$prop' because it is important in higher priority settings and not marked important in source" | ||
| return | ||
| } | ||
| if ($dst.Contains($prop) -and $src.PSObject.Properties.Name -contains $prop) { | ||
| # Remove the property from the destination object only if it also exists in the source object. The property will be re-added with the new value later on. | ||
| OutputDebug "Overwriting setting $prop" | ||
|
|
@@ -37,7 +43,7 @@ function MergeCustomObjectIntoOrderedDictionary { | |
| $src.PSObject.Properties.GetEnumerator() | ForEach-Object { | ||
| $prop = $_.Name | ||
|
|
||
| # Skip overwriteSettings property as it's only used to remove settings from the destination object and is specific to the source object | ||
| # Skip overwriteSettings property as it's only used for configuration, not actual settings | ||
| if ($prop -eq "overwriteSettings") { | ||
| return | ||
| } | ||
|
|
@@ -62,7 +68,7 @@ function MergeCustomObjectIntoOrderedDictionary { | |
| # If the property exists in the source object, but is of a different type, throw an error | ||
| # If the property exists in the source object: | ||
| # If the property is an Object, call this function recursively to merge values | ||
| # If the property is an Object[], merge the arrays | ||
| # If the property is an Object[], merge the arrays (even if important - arrays always merge) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's not intuitive, why would arrays always merge even the setting in the source is marked as important?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I wasn't sure about that either, but I couldn't think of a better solution. I think it's important for arrays that the “important” values are preserved. For example, with the buildModes, the “important” ones should always be executed, but also it should be possible to add others. If I were to change the behavior here so that they aren’t always merged, that would mean that whenever I want to add buildModes, I’d also have to set
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the biggest problem with this is that some settings are dependent of each other - shell, runs-on, useCompilerFolder, etc. and it becomes very hard to understand why a setting, which I explicitly set in my project now suddenly doesn't work because somebody has added a higher level importantSettings. I would rather have a setup where a higher level could specify importantSettings - but it didn't affect the merge of settings, but instead - it would give an error when the setting was overridden lower down - then people can fix this - meaning a non-intrusive behavior really. Another thing we could implement was a dump after readsettings, where it would print out specifically which settings where read from which files: artifact - taken from project setting xxxx That would help people setup the right settings.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Freddy Kristiansen (@freddydk) Your comment isn't about how to handle arrays, but is more of a general point, right?
If we look at Use Case 2 from the PR description, this would be disadvantageous. If I have to define the country in the repo again for the build mode, I might as well skip entering it in the organization settings altogether. Furthermore, I think that anyone who is allowed to edit earlier settings (org + repo) should have the foresight and sense of responsibility to be able to override the repo settings.
i could do that, Maria Zhelezova (@mazhelez) should i? |
||
| # If the property is a simple type, replace the value in the destination object with the value from the source object | ||
| @($dst.Keys) | ForEach-Object { | ||
| $prop = $_ | ||
|
|
@@ -71,6 +77,13 @@ function MergeCustomObjectIntoOrderedDictionary { | |
| $srcProp = $src."$prop" | ||
| $dstPropType = $dstProp.GetType().Name | ||
| $srcPropType = $srcProp.GetType().Name | ||
|
|
||
| # For non-array properties: skip if this setting is marked as important from higher priority source, | ||
| # unless the lower-priority source also marks this property as important. | ||
| if ($dstImportantSettings -contains $prop -and $srcPropType -ne "Object[]" -and $srcImportantSettings -notcontains $prop) { | ||
| OutputDebug "Skipping important setting '$prop' marked from higher priority source (non-array type)" | ||
| return | ||
| } | ||
| if ($srcPropType -eq "PSCustomObject" -and $dstPropType -eq "OrderedDictionary") { | ||
| MergeCustomObjectIntoOrderedDictionary -dst $dst."$prop" -src $srcProp | ||
| } | ||
|
|
@@ -262,7 +275,8 @@ function GetDefaultSettings | |
| "filesToInclude" = @() | ||
| "filesToExclude" = @() | ||
| } | ||
| "postponeProjectInBuildOrder" = $false | ||
| "postponeProjectInBuildOrder" = $false | ||
| "importantSettings" = @() | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -483,11 +497,19 @@ function ReadSettings { | |
| } | ||
| } | ||
|
|
||
| foreach($settingsObject in $settingsObjects) { | ||
| $currentImportantSettings = @() | ||
| foreach ($settingsObject in $settingsObjects) { | ||
| $settingsJson = $settingsObject.Settings | ||
| if ($settingsJson) { | ||
| OutputDebug "Applying settings from $($settingsObject.Source) ($($settingsObject.Type))" | ||
| MergeCustomObjectIntoOrderedDictionary -dst $settings -src $settingsJson | ||
| $srcImportantSettings = @() | ||
| if ($settingsJson.PSObject.Properties.Name -contains "importantSettings") { | ||
| $srcImportantSettings = @($settingsJson.importantSettings) | ||
| } | ||
| MergeCustomObjectIntoOrderedDictionary -dst $settings -src $settingsJson -srcImportantSettings $srcImportantSettings -dstImportantSettings $currentImportantSettings | ||
| if ($settingsJson.PSObject.Properties.Name -contains "importantSettings") { | ||
| $currentImportantSettings = @($currentImportantSettings + $srcImportantSettings | Select-Object -Unique) | ||
| } | ||
| if ($settingsJson.PSObject.Properties.Name -eq "ConditionalSettings") { | ||
| foreach($conditionalSetting in $settingsJson.ConditionalSettings) { | ||
| if ("$conditionalSetting" -ne "") { | ||
|
|
@@ -509,7 +531,14 @@ function ReadSettings { | |
| } | ||
| if ($conditionMet) { | ||
| OutputDebug "Applying conditional settings for $($conditions -join ", ")" | ||
| MergeCustomObjectIntoOrderedDictionary -dst $settings -src $conditionalSetting.settings | ||
| $srcImportantSettings = @() | ||
| if ($conditionalSetting.settings.PSObject.Properties.Name -contains "importantSettings") { | ||
| $srcImportantSettings = @($conditionalSetting.settings.importantSettings) | ||
| } | ||
| MergeCustomObjectIntoOrderedDictionary -dst $settings -src $conditionalSetting.settings -srcImportantSettings $srcImportantSettings -dstImportantSettings $currentImportantSettings | ||
| if ($conditionalSetting.settings.PSObject.Properties.Name -contains "importantSettings") { | ||
| $currentImportantSettings = @($currentImportantSettings + $srcImportantSettings | Select-Object -Unique) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does
importantSettingsneed to be transferred here? I see they are accumulated in$currentImportantSettingsUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is unnecessary from a technical standpoint, but I still think it makes sense to include them so that later on e.g., during debugging it’s clear why a setting was inherited in a certain way.
So far, we’ve been doing this with conditional settings as well. These remain part of the settings indefinitely, even after they’ve been completely resolved.
I've also considered no longer accumulating the $currentImportantSettings outside of the MergeCustomObjectIntoOrderedDictionary function, but instead just accessing the property within the function itself.
However, I think it's a cleaner design to keep the control state separate from the data state.
What do you think, keep the cleaner design or should we try to remove
$currentImportantSettings?