Skip to content

Add support for importantSettings to override lower priority settings - #2293

Open
ChrisBlankDe wants to merge 9 commits into
microsoft:mainfrom
ChrisBlankDe:importantSettings
Open

Add support for importantSettings to override lower priority settings#2293
ChrisBlankDe wants to merge 9 commits into
microsoft:mainfrom
ChrisBlankDe:importantSettings

Conversation

@ChrisBlankDe

@ChrisBlankDe ChrisBlankDe commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

❔What, Why & How

This pull request introduces a new importantSettings mechanism that allows you to protect specific settings from being overridden by lower-priority sources in the AL-Go settings hierarchy. The implementation ensures that settings marked as important can only be overridden by lower-priority sources if those sources also explicitly mark the same setting as important. This applies to both standard and conditional settings, and integrates with the existing overwriteSettings mechanism. Documentation and schema files are updated to reflect this new feature.

Important settings protection:

  • Added support for an importantSettings array in settings files, enabling top-level settings to be protected from non-important overrides by lower-priority sources. The merging logic in MergeCustomObjectIntoOrderedDictionary was updated to enforce this rule for both direct and conditional settings, and to ensure that array settings are always merged. [1] [2] [3] [4]

Schema and documentation updates:

  • Updated settings.schema.json to add the importantSettings property and clarified the behavior of overwriteSettings when important settings are involved. [1] [2] [3] [4]
  • Added detailed explanations, examples, and usage notes for importantSettings in RELEASENOTES.md and Scenarios/settings.md. [1] [2]

Other improvements:

  • Added importantSettings to the default settings returned by GetDefaultSettings.
  • Minor parameter and comment cleanups for consistency in PowerShell scripts. [1] [2] [3] [4]

These changes make it easier to enforce organization- or repository-level settings and prevent accidental overrides from lower-priority sources, while maintaining flexibility for advanced scenarios.

Use Case 1

We want to define conditionalSetting for different buildModes as ALGoOrgSettings as we need them in many Repos.
For example we have one BuildMode for MinVersion Validation:

{
    "ConditionalSettings": [
        {
            "buildModes": [
                "minversion"
            ],
            "settings": {
                "artifact": "/sandbox/*//first",
                "nuGetFeedSelectMode": "EarliestMatching"
            }
        }
    ]
}

in the target repo its then enough to define the buildModes:

{
    "buildModes": [
        "default",
        "minversion"
    ]
}

if we add the buildMode to any workflow in the repo/project the artifact get overwritten by the repo/project setting.
If we define the artifact setting in conditional setting as important, this overwrite wouldnt happen.

Use Case 2

Went to define a buildmode for testing isv solution against different localizations

{
    "ConditionalSettings": [
        {
            "buildModes": [
                "at"
            ],
            "settings": {
                "country": "at"
            }
        }
    ]
}

in the target repo its then enough to define the buildModes:

{
    "country": "de",
    "buildModes": [
        "default",
        "at"
    ]
}

the country of this build mode is overwritten by the country defined in the repo.
However, we can't omit the “country” definition from the repo settings because there are solutions that need to be built against ‘w1’ and others that need to be built against ‘de’.

Related to issue: #

✅ Checklist

  • Add tests (E2E, unit tests)
  • Update RELEASENOTES.md
  • Update documentation (e.g. for new settings or scenarios)
  • Add telemetry

@ChrisBlankDe
ChrisBlankDe marked this pull request as ready for review June 26, 2026 13:53
@ChrisBlankDe
ChrisBlankDe requested a review from a team as a code owner June 26, 2026 13:53
Copilot AI review requested due to automatic review settings June 26, 2026 13:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This pull request adds an importantSettings mechanism to AL-Go’s settings merge logic so selected settings can be “protected” during multi-source settings resolution, and documents the new behavior across schema + docs + release notes with accompanying Pester tests.

Changes:

  • Extend settings merge logic to recognize an importantSettings list and prevent overriding of protected (non-array) settings.
  • Add Pester tests covering important setting protection, interaction with arrays/overwriteSettings, and conditional settings.
  • Update schema and documentation (settings.md, RELEASENOTES.md) to describe the new setting and expected behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
Actions/.Modules/ReadSettings.psm1 Implements important-setting tracking in the merge pipeline.
Actions/.Modules/settings.schema.json Adds importantSettings to the settings schema with defaults/description.
Tests/ReadSettings.Test.ps1 Adds test coverage for the important-setting merge semantics.
Scenarios/settings.md Documents importantSettings behavior and examples.
RELEASENOTES.md Adds release note entry describing the new capability.

Comment thread Actions/.Modules/ReadSettings.psm1 Outdated
Comment thread Actions/.Modules/ReadSettings.psm1 Outdated
Comment thread Tests/ReadSettings.Test.ps1 Outdated
Comment thread Tests/ReadSettings.Test.ps1 Outdated
Comment thread Scenarios/settings.md Outdated
Comment thread RELEASENOTES.md
Comment thread Actions/.Modules/settings.schema.json Outdated
@ChrisBlankDe
ChrisBlankDe marked this pull request as draft June 26, 2026 14:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Comment thread Actions/.Modules/ReadSettings.psm1
Comment thread Actions/.Modules/ReadSettings.psm1
Comment thread Actions/.Modules/ReadSettings.psm1
Comment thread Actions/.Modules/ReadSettings.psm1
Comment thread Tests/ReadSettings.Test.ps1 Outdated
Comment thread Tests/ReadSettings.Test.ps1
@ChrisBlankDe
ChrisBlankDe marked this pull request as ready for review June 29, 2026 13:34
@mazhelez

mazhelez commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Interesting concept. We'd have to give it a bit of a thought, as it goes a bit against the way "important" settings are defined.
In AL-Go, project settings are higher in priority than org settings, for example.

For now, I can't tell I like importantSettings because it might mean a lot of things. How about preventOverwrite, or protectedSettings?

@ChrisBlankDe

Copy link
Copy Markdown
Contributor Author

I missed to add the use-case to description. added now. I think this will help understand what we're trying to achieve with this feature.

When naming it, we got inspired by the !important rule in CSS, since we want to implement a similar concept.

i wouldnt name it preventOverwrite since overwritting is still possible, if the overwriting source is also defined as important. And we already use that name for a special meachanism in handling AL-Go Settings.

Think about what you'd like to call it, and I'll be happy to change it.
In the end, the feature is what matters to me, not the name.

Comment thread RELEASENOTES.md
Comment thread RELEASENOTES.md
@mazhelez mazhelez self-assigned this Jul 21, 2026

@mazhelez mazhelez left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Left some comments for you to look at.

Also, let's also address the name of the new setting. I understand the CSS origin, but the syntax is quite different.
Here are my suggestions:

  1. protectedSettings
  2. enforcedSettings

Comment thread Actions/.Modules/ReadSettings.psm1 Outdated
"productId" = ""
"includeDependencies" = @()
"continuousDelivery" = $false
"mainAppFolder" = ""

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please leave the formatting as is.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed.
code was autoformatted by vscode.
maybe we could add autoformat settings into the repo. to avoid this in future.

# 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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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 overwriteSettings: ["buildModes"] and include the earlier-level values in the overwrite. I think this is a bit clunky, since, in my opinion, “overwrite” should only be used to remove values from earlier levels.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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
runs-on - taken from .github/AL-Go-settings.json
.....

That would help people setup the right settings.
Adding overwrite and important settings seems to only complicate things, sorry.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@freddydk Your comment isn't about how to handle arrays, but is more of a general point, right?

it would give an error when the setting was overridden lower down

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.

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
runs-on - taken from .github/AL-Go-settings.json

i could do that, @mazhelez should i?
print always to the logs or just with ::debug:: annotation?

$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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Does importantSettings need to be transferred here? I see they are accumulated in $currentImportantSettings

@ChrisBlankDe ChrisBlankDe Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

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?

Copilot AI review requested due to automatic review settings July 28, 2026 13:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (5)

Scenarios/settings.md:324

  • This explanation reverses the merge order described above: line 13 says later-applied sources win, while this paragraph calls the organization source “higher priority” and the later project source “lower priority.” In that terminology, a lower-priority source would not override the value in the first place. Describe the feature in terms of earlier/later application order so users can tell which values are protected.
By default, AL-Go follows a standard settings hierarchy where settings from higher priority levels (closer to deployment) override settings from lower priority levels. However, you can mark specific settings as **important** to protect them from being overridden by lower priority settings using the `importantSettings` array.

When a setting is marked as important at a higher level in the hierarchy, it cannot be overridden by non-important values from lower priority levels. If a lower-priority source also marks the same setting as important, then the lower-priority value is allowed to override.

Actions/.Modules/settings.schema.json:402

  • This wording implies that nested deliverToAppSource members can individually be marked important, but importance is tracked only for top-level names and the recursive merge receives no importance lists (ReadSettings.psm1:89). If this describes the parent-level gate, name deliverToAppSource explicitly so users do not try to add includeDependencies to importantSettings.
                    "description": "An array of settings to be overwritten by the current deliverToAppSource setting. If a setting is marked as important in higher-priority settings, overwrite is ignored unless the current source also marks that same setting as important. See https://aka.ms/ALGoSettings#overwriteSettings"

Actions/.Modules/settings.schema.json:657

  • This wording implies that nested alDoc members can individually be marked important, but importance is tracked only for top-level names and the recursive merge receives no importance lists (ReadSettings.psm1:89). Clarify that the gate applies to the top-level alDoc setting rather than names such as includeProjects.
                "description": "An array of settings to be overwritten by the current alDoc setting. If a setting is marked as important in higher-priority settings, overwrite is ignored unless the current source also marks that same setting as important. See https://aka.ms/ALGoSettings#overwriteSettings"

Actions/.Modules/settings.schema.json:687

  • This wording implies that nested commitOptions members can individually be marked important, but importance is tracked only for top-level names and the recursive merge receives no importance lists (ReadSettings.psm1:89). Clarify that the gate applies to the top-level commitOptions setting rather than pullRequestLabels.
                    "description": "An array of settings to be overwritten by the current commitOptions setting. If a setting is marked as important in higher-priority settings, overwrite is ignored unless the current source also marks that same setting as important. See https://aka.ms/ALGoSettings#overwriteSettings"

Actions/.Modules/ReadSettings.psm1:86

  • This guard also changes merging for complex object settings such as deliverToAppSource, alDoc, and commitOptions, but the added tests cover only scalar and array values. Add Pester cases showing that a non-important source cannot modify an important object and that a source marking the parent object important can merge or overwrite it; otherwise regressions in the recursive path are untested.
            # 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

Copilot AI review requested due to automatic review settings July 28, 2026 14:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

Scenarios/settings.md:324

  • The priority terminology is reversed relative to AL-Go’s existing merge contract: line 13 says the last-applied source wins, and ReadSettings documents customSettings (applied last) as having the highest precedence. Organization settings are therefore earlier/lower-precedence sources, while project/workflow/custom settings are later/higher-precedence sources. Calling the organization source “higher priority” makes this feature sound redundant and can lead users to place protection at the wrong level. Please describe this in terms of earlier and later sources (and align the same wording in the release notes and schema descriptions).
By default, AL-Go follows a standard settings hierarchy where settings from higher priority levels (closer to deployment) override settings from lower priority levels. However, you can mark specific settings as **important** to protect them from being overridden by lower priority settings using the `importantSettings` array.

When a setting is marked as important at a higher level in the hierarchy, it cannot be overridden by non-important values from lower priority levels. If a lower-priority source also marks the same setting as important, then the lower-priority value is allowed to override.

Scenarios/settings.md:395

  • The new setting documentation does not state its default/required status or which settings-loading paths honor it. Add that importantSettings is optional, defaults to [], and is honored whenever AL-Go merges sources through ReadSettings, so users have the metadata required for a new AL-Go setting.
> _**Note**_: `importantSettings` is an array of setting names that should be protected from non-important overrides from lower priority settings. If the same setting is marked as important at both levels, the source (lower-priority) value is allowed to override the destination value. Only top-level setting names can be marked as important; nested properties within complex objects cannot be individually marked as important. Array settings marked as important are still merged with lower-priority arrays. `overwriteSettings` can force replacement for important settings only when the source also marks that same setting as important.

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.

5 participants