From 7bea84659473fe5789a8a11477bd6e62ad52dc1d Mon Sep 17 00:00:00 2001 From: Yingjie Date: Tue, 6 Oct 2026 16:35:51 +0800 Subject: [PATCH] fix: apply later-library policy (set) definitions on library overwrite --- alzlib.go | 7 +- alzlib_test.go | 192 ++++++++++++++++++ cmd/alzlibtool/command/cache/create.go | 25 ++- cmd/alzlibtool/command/check/library.go | 11 +- cmd/alzlibtool/command/check/library_test.go | 13 +- cmd/alzlibtool/command/document/library.go | 18 +- .../command/generate/architecture.go | 19 +- .../generate/architecture_overwrite_test.go | 10 +- 8 files changed, 240 insertions(+), 55 deletions(-) diff --git a/alzlib.go b/alzlib.go index 5ff5a23..3492c95 100644 --- a/alzlib.go +++ b/alzlib.go @@ -159,7 +159,7 @@ func (az *AlzLib) AddPolicyDefinitions(pds ...*assets.PolicyDefinition) error { if pdvc, exists := az.policyDefinitions[*pd.Name]; exists { if err := pdvc.Add(pd, az.Options.AllowOverwrite); err != nil { - merr = multierror.Append(merr, pdvc.Upsert(pdvc, az.Options.AllowOverwrite)) + merr = multierror.Append(merr, err) } continue @@ -1484,9 +1484,10 @@ func (az *AlzLib) fetchLatestReferencedPolicyDefinition( func (az *AlzLib) addPolicyAndRoleAssets(res *processor.Result) error { var merr error + // Merge into the stored collection so that later libraries win. for k, v := range res.PolicyDefinitions { if pdv, exists := az.policyDefinitions[k]; exists { - if err := v.Upsert(pdv, az.Options.AllowOverwrite); err != nil { + if err := pdv.Upsert(v, az.Options.AllowOverwrite); err != nil { merr = multierror.Append(merr, fmt.Errorf("policy definition %s: %w", k, err)) } @@ -1504,7 +1505,7 @@ func (az *AlzLib) addPolicyAndRoleAssets(res *processor.Result) error { for k, v := range res.PolicySetDefinitions { if psdv, exists := az.policySetDefinitions[k]; exists { - if err := v.Upsert(psdv, az.Options.AllowOverwrite); err != nil { + if err := psdv.Upsert(v, az.Options.AllowOverwrite); err != nil { merr = multierror.Append(merr, fmt.Errorf("policy set definition %s: %w", k, err)) } diff --git a/alzlib_test.go b/alzlib_test.go index ce52028..cb53439 100644 --- a/alzlib_test.go +++ b/alzlib_test.go @@ -99,6 +99,198 @@ func TestAddPolicyAndRoleAssetsAllowsDuplicateVersions(t *testing.T) { } } +// libraryDefinition describes a definition with the given name supplied by one library. +type libraryDefinition struct { + version *string + description string +} + +// TestAddPolicyAndRoleAssetsLaterLibraryDefinitions loads each definition as a separate +// library, in order, and checks which versions end up in the AlzLib. +func TestAddPolicyAndRoleAssetsLaterLibraryDefinitions(t *testing.T) { + v1, v2 := to.Ptr("1.0.0"), to.Ptr("2.0.0") + + testCases := []struct { + name string + overwrite bool + libs []libraryDefinition + wantErr string + // want maps version to description; the empty key is the versionless definition. + want map[string]string + }{ + { + name: "versionless redefinition with overwrite", + overwrite: true, + libs: []libraryDefinition{{nil, "earlier"}, {nil, "later"}}, + want: map[string]string{"": "later"}, + }, + { + name: "versionless redefinition without overwrite", + overwrite: false, + libs: []libraryDefinition{{nil, "earlier"}, {nil, "later"}}, + wantErr: "cannot add overwrite versionless definition", + }, + { + name: "same version redefinition with overwrite", + overwrite: true, + libs: []libraryDefinition{{v1, "earlier"}, {v1, "later"}}, + want: map[string]string{"1.0.0": "later"}, + }, + { + name: "same version redefinition without overwrite", + overwrite: false, + libs: []libraryDefinition{{v1, "earlier"}, {v1, "later"}}, + wantErr: "already exists and the new definition is different", + }, + { + name: "new version without overwrite", + overwrite: false, + libs: []libraryDefinition{{v1, "first"}, {v2, "second"}}, + want: map[string]string{"1.0.0": "first", "2.0.0": "second"}, + }, + { + name: "new version with overwrite", + overwrite: true, + libs: []libraryDefinition{{v1, "first"}, {v2, "second"}}, + want: map[string]string{"1.0.0": "first", "2.0.0": "second"}, + }, + { + name: "last of three libraries wins", + overwrite: true, + libs: []libraryDefinition{{nil, "first"}, {nil, "second"}, {nil, "third"}}, + want: map[string]string{"": "third"}, + }, + { + name: "versionless onto versioned", + overwrite: true, + libs: []libraryDefinition{{v1, "earlier"}, {nil, "later"}}, + wantErr: "cannot add versionless definition", + }, + { + name: "versioned onto versionless", + overwrite: true, + libs: []libraryDefinition{{nil, "earlier"}, {v1, "later"}}, + wantErr: "cannot add versioned definition", + }, + } + + for _, tc := range testCases { + t.Run("policy definition/"+tc.name, func(t *testing.T) { + az := NewAlzLib(&Options{AllowOverwrite: tc.overwrite, Parallelism: defaultParallelism}) + + err := addDefinitionsAsLibraries(t, az, assets.NewPolicyDefinitionVersions, + func(res *processor.Result, c *assets.PolicyDefinitionVersions) { + res.PolicyDefinitions["test"] = c + }, + func(d libraryDefinition) *assets.PolicyDefinition { + pd := testPolicyDefinition(t, "test", "") + pd.Properties.Version = d.version + pd.Properties.Description = to.Ptr(d.description) + + return pd + }, + tc.libs..., + ) + if tc.wantErr != "" { + require.ErrorContains(t, err, tc.wantErr) + return + } + + require.NoError(t, err) + assertDefinitionDescriptions(t, az.policyDefinitions["test"], tc.want, + func(pd *assets.PolicyDefinition) string { return *pd.Properties.Description }) + }) + + t.Run("policy set definition/"+tc.name, func(t *testing.T) { + az := NewAlzLib(&Options{AllowOverwrite: tc.overwrite, Parallelism: defaultParallelism}) + + err := addDefinitionsAsLibraries(t, az, assets.NewPolicySetDefinitionVersions, + func(res *processor.Result, c *assets.PolicySetDefinitionVersions) { + res.PolicySetDefinitions["test"] = c + }, + func(d libraryDefinition) *assets.PolicySetDefinition { + psd := testPolicySetDefinition(t, "test", "") + psd.Properties.Version = d.version + psd.Properties.Description = to.Ptr(d.description) + + return psd + }, + tc.libs..., + ) + if tc.wantErr != "" { + require.ErrorContains(t, err, tc.wantErr) + return + } + + require.NoError(t, err) + assertDefinitionDescriptions(t, az.policySetDefinitions["test"], tc.want, + func(psd *assets.PolicySetDefinition) string { return *psd.Properties.Description }) + }) + } +} + +func addDefinitionsAsLibraries[T assets.Versioned]( + t *testing.T, + az *AlzLib, + newVersions func() *assets.VersionedPolicyCollection[T], + setResult func(*processor.Result, *assets.VersionedPolicyCollection[T]), + newDefinition func(libraryDefinition) T, + libs ...libraryDefinition, +) error { + t.Helper() + + for _, lib := range libs { + c := newVersions() + require.NoError(t, c.Add(newDefinition(lib), false)) + + res := processor.NewResult() + setResult(res, c) + + if err := az.addPolicyAndRoleAssets(res); err != nil { + return err + } + } + + return nil +} + +func assertDefinitionDescriptions[T assets.Versioned]( + t *testing.T, + c *assets.VersionedPolicyCollection[T], + want map[string]string, + description func(T) string, +) { + t.Helper() + require.NotNil(t, c) + + wantVersioned := 0 + + for ver, wantDesc := range want { + var verPtr *string + if ver != "" { + verPtr = to.Ptr(ver) + wantVersioned++ + } + + got, err := c.GetVersionStrict(verPtr) + require.NoError(t, err, "version %q", ver) + assert.Equal(t, wantDesc, description(got), "version %q", ver) + } + + assert.Len(t, c.Versions(), wantVersioned) +} + +func TestAddPolicyDefinitionsReturnsAddError(t *testing.T) { + az := NewAlzLib(nil) + require.NoError(t, az.AddPolicyDefinitions(testPolicyDefinition(t, "test", "1.0.0"))) + + versionless := testPolicyDefinition(t, "test", "") + versionless.Properties.Version = nil + + err := az.AddPolicyDefinitions(versionless) + require.ErrorContains(t, err, "cannot add versionless definition") +} + // Test_NewAlzLib_noDir tests the creation of a new AlzLib when supplied with a path // that does not exist. // The error details are checked for the expected error message. diff --git a/cmd/alzlibtool/command/cache/create.go b/cmd/alzlibtool/command/cache/create.go index ef9d139..b0164fe 100644 --- a/cmd/alzlibtool/command/cache/create.go +++ b/cmd/alzlibtool/command/cache/create.go @@ -40,12 +40,12 @@ A --library value may be either a local path (e.g. ./mylib) or an ALZ Library re the form @, e.g. platform/alz@2026.01.3. The flag may be specified multiple times to combine several libraries; their dependencies are fetched recursively. -Use --library-overwrite-enabled to apply the current library-overwrite behavior while combining -libraries. Policy assignments, role definitions, archetypes, architectures and policy default -values from later libraries replace those of earlier libraries in full, they are not merged -field by field. Policy definitions and policy set definitions are not replaced: the earlier -library's version is kept and the flag only suppresses the duplicate error. Duplicate archetype -override names remain an error regardless of this flag. +Use --library-overwrite-enabled to let later libraries redefine assets of earlier libraries +when combining them. Policy assignments, role definitions, archetypes, architectures and +policy default values from later libraries replace those of earlier libraries in full, they +are not merged field by field. Policy definitions and policy set definitions are replaced per +version: a redefined version replaces the earlier library's, other versions are kept. +Duplicate archetype override names remain an error regardless of this flag. Use --from-cache to seed from an existing cache file (requires --library and --architecture). Definitions already present in the seed cache are used directly and not re-fetched from Azure, @@ -94,8 +94,8 @@ to update a cache in-place.`, if libraryOverwriteEnabled { // stderr only, so verbose stdout stays scriptable. cmd.PrintErrln( - "Warning: policy (set) definition redefinitions retain the earlier library's version, " + - "and duplicate archetype override names remain an error.") + "Warning: assets redefined by later libraries replace earlier ones in full, not field " + + "by field, and duplicate archetype override names remain an error.") } // Read the seed cache BEFORE opening the output file, because --from-cache @@ -274,9 +274,8 @@ func init() { createCmd.Flags(). Bool( "library-overwrite-enabled", false, - "Apply the current library-overwrite behavior while combining libraries. Policy "+ - "assignments, role definitions, archetypes, architectures and policy default values from "+ - "later libraries are replaced in full, not merged field by field. Policy (set) definitions "+ - "keep the earlier library's version; for those the flag only suppresses the duplicate "+ - "error. Requires --library and --architecture.") + "Allow later libraries to redefine assets of earlier libraries. Redefined assets are "+ + "replaced in full, not merged field by field; policy (set) definitions are replaced per "+ + "version. Duplicate archetype override names remain an error. Requires --library and "+ + "--architecture.") } diff --git a/cmd/alzlibtool/command/check/library.go b/cmd/alzlibtool/command/check/library.go index 66d3f84..5c19c3d 100644 --- a/cmd/alzlibtool/command/check/library.go +++ b/cmd/alzlibtool/command/check/library.go @@ -29,8 +29,8 @@ initialization with an "already exists in the library" error and no checks are r Assets are replaced in full, they are not merged field by field, so the redefining file must contain the complete asset. Policy assignments, role definitions, archetypes, architectures and policy default values are replaced by this member's version. Policy -definitions and policy set definitions are not: for those the flag only suppresses the -duplicate error and the dependency version is kept. Duplicate archetype override names +definitions and policy set definitions are replaced per version: a redefined version +replaces the dependency's, other versions are kept. Duplicate archetype override names remain an error regardless of this flag.`, Args: cobra.ExactArgs(1), Run: func(cmd *cobra.Command, args []string) { @@ -131,8 +131,7 @@ func init() { libraryCmd.Flags(). Bool( "library-overwrite-enabled", false, - "Allow this library member to redefine assets provided by its dependencies. Policy "+ - "assignments, role definitions, archetypes, architectures and policy default values are "+ - "replaced in full, not merged field by field. Policy (set) definitions keep the "+ - "dependency version; for those the flag only suppresses the duplicate error.") + "Allow this library member to redefine assets provided by its dependencies. Redefined "+ + "assets are replaced in full, not merged field by field; policy (set) definitions are "+ + "replaced per version. Duplicate archetype override names remain an error.") } diff --git a/cmd/alzlibtool/command/check/library_test.go b/cmd/alzlibtool/command/check/library_test.go index 89e1309..8570ce4 100644 --- a/cmd/alzlibtool/command/check/library_test.go +++ b/cmd/alzlibtool/command/check/library_test.go @@ -102,12 +102,11 @@ func Test_libraryCmd_noConflictWithoutFlag(t *testing.T) { assert.Equal(t, 0, code, "output: %s", out) } -// Test_libraryCmd_policyDefinitionOverrideNotApplied pins current engine behaviour: for policy -// (set) definitions the flag only suppresses the duplicate error, the dependency version is -// kept. The pdmember version of dependency-pd adds the parameter its defaults require, so the -// defaults check failing proves the member version was discarded. -func Test_libraryCmd_policyDefinitionOverrideNotApplied(t *testing.T) { +// Test_libraryCmd_policyDefinitionOverrideApplied: the pdmember version of dependency-pd adds +// the parameter its defaults require, so passing the defaults check proves the member version +// replaced the dependency version. +func Test_libraryCmd_policyDefinitionOverrideApplied(t *testing.T) { out, code := runCheckLibrary(t, overwritePdMemberLib, "--offline", "--library-overwrite-enabled") - assert.Equal(t, 1, code) - assert.Contains(t, out, "not present in the referenced definition") + assert.Equal(t, 0, code, "output: %s", out) + assert.NotContains(t, out, "not present in the referenced definition") } diff --git a/cmd/alzlibtool/command/document/library.go b/cmd/alzlibtool/command/document/library.go index ba399ed..70f02c7 100644 --- a/cmd/alzlibtool/command/document/library.go +++ b/cmd/alzlibtool/command/document/library.go @@ -20,10 +20,10 @@ Use --library-overwrite-enabled when this library member intentionally redefines are also provided by its dependencies. Without it, documentation generation fails during library initialization with an "already exists in the library" error. -The generated documentation follows the current overwrite semantics: policy assignments, role -definitions, archetypes, architectures and policy default values are replaced in full by this -member's version. Policy definitions and policy set definitions are not, the dependency -version is kept. Duplicate archetype override names remain an error regardless of this flag.`, +The generated documentation uses this member's version of redefined assets, replaced in full +rather than merged field by field. Policy definitions and policy set definitions are replaced +per version: a redefined version replaces the dependency's, other versions are kept. +Duplicate archetype override names remain an error regardless of this flag.`, Args: cobra.ExactArgs(1), Run: func(cmd *cobra.Command, args []string) { thislib := alzlib.NewCustomLibraryReference(args[0]) @@ -46,8 +46,7 @@ version is kept. Duplicate archetype override names remain an error regardless o if libraryOverwriteEnabled { // stderr only, so the Markdown on stdout stays usable in a pipeline. cmd.PrintErrln( - "Warning: generated documentation follows the existing overwrite semantics; " + - "policy (set) definition redefinitions retain the dependency version, and " + + "Warning: redefined assets replace the dependency's in full, not field by field, and " + "duplicate archetype override names remain an error.") } @@ -63,8 +62,7 @@ func init() { documentLibraryBaseCmd.Flags(). Bool( "library-overwrite-enabled", false, - "Document the library using the current overwrite semantics. Policy assignments, role "+ - "definitions, archetypes, architectures and policy default values are replaced in full, "+ - "not merged field by field. Policy (set) definitions keep the dependency version; for "+ - "those the flag only suppresses the duplicate error.") + "Allow this library member to redefine assets provided by its dependencies. Redefined "+ + "assets are replaced in full, not merged field by field; policy (set) definitions are "+ + "replaced per version. Duplicate archetype override names remain an error.") } diff --git a/cmd/alzlibtool/command/generate/architecture.go b/cmd/alzlibtool/command/generate/architecture.go index bd6bce6..73573f1 100644 --- a/cmd/alzlibtool/command/generate/architecture.go +++ b/cmd/alzlibtool/command/generate/architecture.go @@ -34,11 +34,11 @@ Use --library-overwrite-enabled when the library member intentionally redefines are also provided by its dependencies. Without it, generation fails during library initialization with an "already exists in the library" error. -The generated output follows the current overwrite semantics: policy assignments, role -definitions, archetypes, architectures and policy default values are replaced in full by the -member's version. Policy definitions and policy set definitions are not, the dependency -version is kept. Duplicate archetype override names remain an error regardless of this flag. -Verify the generated output before deploying it.`, +The generated output uses the member's version of redefined assets, replaced in full rather +than merged field by field. Policy definitions and policy set definitions are replaced per +version: a redefined version replaces the dependency's, other versions are kept. Duplicate +archetype override names remain an error regardless of this flag. Verify the generated +output before deploying it.`, Args: cobra.ExactArgs(RequiredArchitectureArgs), Run: func(cmd *cobra.Command, args []string) { thisLib := alzlib.NewCustomLibraryReference(args[0]) @@ -63,7 +63,7 @@ Verify the generated output before deploying it.`, if libraryOverwriteEnabled { // stderr only, so the JSON on stdout stays parseable in a pipeline. cmd.PrintErrln( - "Warning: policy (set) definition redefinitions retain the dependency version, and " + + "Warning: redefined assets replace the dependency's in full, not field by field, and " + "duplicate archetype override names remain an error. Verify the generated output.") } @@ -195,8 +195,7 @@ func init() { generateArchitectureBaseCmd.Flags(). Bool( "library-overwrite-enabled", false, - "Apply the current library-overwrite behavior while generating output. Policy "+ - "assignments, role definitions, archetypes, architectures and policy default values are "+ - "replaced in full, not merged field by field. Policy (set) definitions keep the "+ - "dependency version; for those the flag only suppresses the duplicate error.") + "Allow the library member to redefine assets provided by its dependencies. Redefined "+ + "assets are replaced in full, not merged field by field; policy (set) definitions are "+ + "replaced per version. Duplicate archetype override names remain an error.") } diff --git a/cmd/alzlibtool/command/generate/architecture_overwrite_test.go b/cmd/alzlibtool/command/generate/architecture_overwrite_test.go index 4efd77a..fb16ae7 100644 --- a/cmd/alzlibtool/command/generate/architecture_overwrite_test.go +++ b/cmd/alzlibtool/command/generate/architecture_overwrite_test.go @@ -110,9 +110,8 @@ func Test_generateArchitectureCmd_overwriteEnabled(t *testing.T) { } // Test_generateArchitectureCmd_outputAssetContract pins what actually lands in the deployment -// artifact: the member policy assignment wins, but a redefined policy definition still comes -// from the dependency, because the engine keeps the dependency version for policy (set) -// definitions. +// artifact: the member version wins for both the policy assignment and the redefined policy +// definition. func Test_generateArchitectureCmd_outputAssetContract(t *testing.T) { memberOut := t.TempDir() @@ -131,9 +130,8 @@ func Test_generateArchitectureCmd_outputAssetContract(t *testing.T) { definition := readGeneratedAsset(t, pdMemberOut, "dependency-pd") - assert.Contains(t, definition, "PD from dependency", - "policy definitions keep the dependency version, see the flag help") - assert.NotContains(t, definition, "PD from pdmember") + assert.Contains(t, definition, "PD from pdmember") + assert.NotContains(t, definition, "PD from dependency") } func readGeneratedAsset(t *testing.T, dir, name string) string {