Conversation
…ding and GC - cache/cache.go: Add PolicyDefinitionVersionsByName and PolicySetDefinitionVersionsByName per-name lookup methods, avoiding full-map clone for individual lookups - alzlib.go: Extend BuiltInCache interface with new per-name lookup methods - alzlib.go: Add `cache BuiltInCache` field to AlzLib struct - alzlib.go: AddCache now stores only the cache reference (lazy, not eager) - alzlib.go: GetDefinitionsFromAzure defers az.cache = nil so GC can reclaim cache - alzlib.go: getBuiltInPolicies checks cache before Azure; lazy-init Azure client - alzlib.go: getBuiltInPolicySets checks cache before Azure; lazy-init Azure clients - alzlib.go: ensureReferencedPolicyDefinitions removes upfront client creation - alzlib.go: ensureReferencedPolicyDefinition checks AlzLib → cache → Azure in order - alzlib.go: Extract policyDefinitionFromCache/policySetDefinitionFromCache helpers - alzlib_test.go: Update mockBuiltInCache with new interface methods - alzlib_test.go: Replace eager-load tests with lazy-load tests - integrationtest/cache_test.go: Update TestCacheInitWithLibrary for lazy semantics - docs/cache.md: Document lazy loading and GC behaviour Co-authored-by: matt-FFFFFF <16320656+matt-FFFFFF@users.noreply.github.com>
|
@copilot add a way to create a new cache once all defs have been loaded. A method on alzlib perhaps? Should be possible to filter out any custom policies by their type or resource id. I want to give callers the ability to generate a cache based on their specific use case - rather than using the alzlibtool cli to cache EVERYTHING. |
…s constructor - cache/cache.go: Add NewCacheFromDefinitions constructor that builds a Cache from existing PolicyDefinitionVersions/PolicySetDefinitionVersions maps - alzlib.go: Add ExportBuiltInCache() method that collects only built-in and static definitions from the loaded AlzLib state, filtering out custom definitions - alzlib.go: Add isBuiltInPolicyDefinitionVersions / isBuiltInPolicySetDefinitionVersions helpers for PolicyType-based filtering - alzlib_test.go: Add 5 tests for ExportBuiltInCache covering the filter logic, save/reload round-trip, empty state, nil PolicyType, and static type Co-authored-by: matt-FFFFFF <16320656+matt-FFFFFF@users.noreply.github.com>
Done in 73bbd44. Added two new APIs:
az := alzlib.NewAlzLib(nil)
az.AddPolicyClient(cf)
// Init loads library, GetDefinitionsFromAzure fetches only referenced built-ins
az.Init(ctx, lib)
deployment.NewHierarchy(az).FromArchitecture(ctx, "alz", subID, location)
// Export only the built-ins that were actually needed
c := az.ExportBuiltInCache()
var buf bytes.Buffer
c.Save(&buf)
// persist buf to a file for reuseDefinitions with |
|
@copilot ok now in the same fashion as alzlibtool generate architecture, extend the alzlibtool cache create to allow the specification of a library path and architecture. Also add --use-cache (or better name) flags to the generate command and to cache create commands. The will allow the efficient update of a cache. It should allow (but not mandate) the same source and destination file. |
…ary+architecture mode to cache create - generate/architecture.go: add --from-cache flag; loads seed cache into AlzLib before Init/FromArchitecture to reduce Azure API calls - cache/create.go: add --library, --architecture, --from-cache, --rootmg, --location flags; new architecture-scoped mode uses alzlib.ExportBuiltInCache() to produce a minimal cache; same path allowed for --from-cache and --output (in-place update) Co-authored-by: matt-FFFFFF <16320656+matt-FFFFFF@users.noreply.github.com>
Done in a4bd09b. Changes:
Example workflows: # Generate a minimal architecture-scoped cache
alzlibtool cache create --library ./my-lib --architecture alz --output alz-cache.json.gz
# Update a cache in-place with definitions from a new/updated architecture
alzlibtool cache create --library ./my-lib --architecture alz \
--from-cache alz-cache.json.gz --output alz-cache.json.gz
# Generate architecture deployment seeded from cache (fewer Azure API calls)
alzlibtool generate architecture ./my-lib alz --from-cache alz-cache.json.gzWarning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the built-in definitions cache integration to lazy-load built-ins on demand (cache → Azure fallback), adds an ExportBuiltInCache API for exporting a minimal cache from the currently-loaded built-ins, and extends alzlibtool to support cache seeding and architecture-scoped cache creation.
Changes:
- Change
AlzLib.AddCachefrom eager population to lazy cache lookup duringGetDefinitionsFromAzure. - Add
AlzLib.ExportBuiltInCache()andcache.NewCacheFromDefinitions(...)to export a minimal built-ins cache. - Extend
alzlibtool cache create/alzlibtool generate architecturewith--from-cache, and add architecture-scoped cache creation flags.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| integrationtest/cache_test.go | Updates integration expectations for lazy cache loading (built-ins appear after architecture processing). |
| docs/cache.md | Updates documentation to describe lazy cache lookup and lifecycle behavior. |
| cmd/alzlibtool/command/generate/architecture.go | Adds --from-cache support to seed built-ins before running architecture generation. |
| cmd/alzlibtool/command/cache/create.go | Adds architecture-scoped cache creation mode and --from-cache seeding for incremental updates. |
| cache/cache.go | Adds constructor to build a cache from already-loaded definition/version maps and O(1) per-name accessors. |
| alzlib_test.go | Updates mocks/tests for lazy cache semantics and adds ExportBuiltInCache coverage. |
| alzlib.go | Implements lazy cache lookup, ExportBuiltInCache, and restructures referenced-definition fetching to avoid eager Azure client construction. |
You can also share your feedback on Copilot code review. Take the survey.
|
@copilot address the UNRESOLVED review comments. All are valid I think. There is a resolved comment that's not applicable. |
- ExportBuiltInCache: deep-copy *PolicyDefinitionVersions values so AlzLib mutations (e.g. SetAssignPermissionsOnDefinitionParameter) cannot affect the exported cache - GetDefinitionsFromAzure: remove defer az.cache=nil; cache is now kept for AlzLib lifetime (callers release with AddCache(nil)). Fixes bug where only the first management group could use the cache. - policyDefinitionFromCache / policySetDefinitionFromCache: snapshot az.cache under az.mu.RLock to prevent data race with AddCache - ensureReferencedPolicyDefinitions: lazily create DefinitionsClient / DefinitionVersionsClient once per invocation and reuse across all policy-set sub-references instead of creating per cache miss - Add TestExportBuiltInCacheIsDeepCopied and helper testBuiltInPolicyDefinitionWithParam - Update AddCache docstring and docs/cache.md for new cache lifecycle Co-authored-by: matt-FFFFFF <16320656+matt-FFFFFF@users.noreply.github.com>
Done in 3cb6a94. All three unresolved comments addressed:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
You can also share your feedback on Copilot code review. Take the survey.
| // set definitions that are currently loaded in AlzLib. Only definitions with a | ||
| // [armpolicy.PolicyTypeBuiltIn] or [armpolicy.PolicyTypeStatic] policy type are included; | ||
| // custom definitions (loaded from library files) are excluded. | ||
| // |
There was a problem hiding this comment.
ExportBuiltInCache docstring says only PolicyTypeBuiltIn/PolicyTypeStatic are included, but isBuiltInPolicyDefinitionVersions/isBuiltInPolicySetDefinitionVersions also treat a nil PolicyType as built-in and therefore include those definitions. Update the docstring (and/or the referenced comment) so it matches the actual inclusion rules.
| // set definitions that are currently loaded in AlzLib. Only definitions with a | |
| // [armpolicy.PolicyTypeBuiltIn] or [armpolicy.PolicyTypeStatic] policy type are included; | |
| // custom definitions (loaded from library files) are excluded. | |
| // | |
| // set definitions that are currently loaded in AlzLib. Definitions are included when their | |
| // policy type is [armpolicy.PolicyTypeBuiltIn], [armpolicy.PolicyTypeStatic], or is | |
| // unspecified/nil (which is treated as built-in); custom definitions (loaded from library | |
| // files) are excluded. |
| if az.clients.policyClient == nil { | ||
| return errors.New("Alzlib.getBuiltInPolicies: policy client not set") | ||
| } | ||
|
|
||
| // Lazily initialise clients on first Azure call. | ||
| if client == nil { | ||
| versionedClient = az.clients.policyClient.NewDefinitionVersionsClient() | ||
| client = az.clients.policyClient.NewDefinitionsClient() |
There was a problem hiding this comment.
getBuiltInPolicies reads az.clients.policyClient without holding az.mu, while AddPolicyClient writes it under az.mu.Lock(). This can still produce a data race if GetDefinitionsFromAzure runs concurrently with AddPolicyClient. Consider snapshotting the policy client under az.mu.RLock() (similar to the cache snapshot) and using the snapshot for the remainder of the call.
| if az.clients.policyClient == nil { | |
| return errors.New("Alzlib.getBuiltInPolicies: policy client not set") | |
| } | |
| // Lazily initialise clients on first Azure call. | |
| if client == nil { | |
| versionedClient = az.clients.policyClient.NewDefinitionVersionsClient() | |
| client = az.clients.policyClient.NewDefinitionsClient() | |
| az.mu.RLock() | |
| policyClient := az.clients.policyClient | |
| az.mu.RUnlock() | |
| if policyClient == nil { | |
| return errors.New("Alzlib.getBuiltInPolicies: policy client not set") | |
| } | |
| // Lazily initialise clients on first Azure call. | |
| if client == nil { | |
| versionedClient = policyClient.NewDefinitionVersionsClient() | |
| client = policyClient.NewDefinitionsClient() |
| if az.clients.policyClient == nil { | ||
| return errors.New("Alzlib.getBuiltInPolicySets: policy client not set") | ||
| } | ||
|
|
||
| // Lazily initialise clients on first Azure call. | ||
| if client == nil { | ||
| versionedClient = az.clients.policyClient.NewSetDefinitionVersionsClient() | ||
| client = az.clients.policyClient.NewSetDefinitionsClient() |
There was a problem hiding this comment.
getBuiltInPolicySets reads az.clients.policyClient without holding az.mu, while AddPolicyClient writes it under az.mu.Lock(). This can lead to a data race if GetDefinitionsFromAzure (or callers) run concurrently with AddPolicyClient. Consider snapshotting the policy client under az.mu.RLock() and using that local reference for Azure fallbacks.
| if az.clients.policyClient == nil { | |
| return errors.New("Alzlib.getBuiltInPolicySets: policy client not set") | |
| } | |
| // Lazily initialise clients on first Azure call. | |
| if client == nil { | |
| versionedClient = az.clients.policyClient.NewSetDefinitionVersionsClient() | |
| client = az.clients.policyClient.NewSetDefinitionsClient() | |
| az.mu.RLock() | |
| policyClient := az.clients.policyClient | |
| az.mu.RUnlock() | |
| if policyClient == nil { | |
| return errors.New("Alzlib.getBuiltInPolicySets: policy client not set") | |
| } | |
| // Lazily initialise clients on first Azure call. | |
| if client == nil { | |
| versionedClient = policyClient.NewSetDefinitionVersionsClient() | |
| client = policyClient.NewSetDefinitionsClient() |
| // Fall back to Azure. | ||
| if az.clients.policyClient == nil { | ||
| return errors.New("Alzlib.ensureReferencedPolicyDefinition: policy client not set") | ||
| } | ||
|
|
||
| if ref.DefinitionVersion != nil { | ||
| return az.fetchReferencedPolicyDefinitionVersions(ctx, definitionsVersionedClient, resID.Name, setDefinition, ref) | ||
| if *definitionsVersionedClient == nil { | ||
| *definitionsVersionedClient = az.clients.policyClient.NewDefinitionVersionsClient() | ||
| } | ||
|
|
||
| return az.fetchReferencedPolicyDefinitionVersions(ctx, *definitionsVersionedClient, resID.Name, setDefinition, ref) | ||
| } | ||
|
|
||
| if *definitionsClient == nil { | ||
| *definitionsClient = az.clients.policyClient.NewDefinitionsClient() |
There was a problem hiding this comment.
ensureReferencedPolicyDefinition reads az.clients.policyClient without synchronization, but AddPolicyClient writes it under az.mu.Lock(). If callers can invoke these concurrently, this is a data race. Snapshot policyClient under az.mu.RLock() (or otherwise synchronize access) before using it to lazily create the Azure clients.
|
@copilot four more review comments. All seem valid. Please take a look. Ignore the resolved comment threads. These have already been implemented or addressed. |
…xportBuiltInCache docstring - getBuiltInPolicies: snapshot az.clients.policyClient under az.mu.RLock() before the Azure fallback to prevent data race with AddPolicyClient - getBuiltInPolicySets: same fix - ensureReferencedPolicyDefinition: same fix - ExportBuiltInCache: update docstring to accurately reflect that nil PolicyType is also included (treated as built-in), not only PolicyTypeBuiltIn and PolicyTypeStatic Co-authored-by: matt-FFFFFF <16320656+matt-FFFFFF@users.noreply.github.com>
Done in 28aa64c:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #241 +/- ##
==========================================
- Coverage 48.43% 47.71% -0.72%
==========================================
Files 54 54
Lines 4590 4824 +234
==========================================
+ Hits 2223 2302 +79
- Misses 2087 2238 +151
- Partials 280 284 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thank you for your contribution @app/copilot-swe-agent! Due to CI/CD limitations with fork PRs and secrets access, I've migrated your changes to #242. All your commits and authorship have been preserved. Please follow the new PR for any updates or requested changes. The new PR will run all required checks and can be merged once approved. |
cache.NewCacheFromDefinitionsconstructorAlzLib.ExportBuiltInCache()methodalzlibtool cache createwith library/architecture flags and --from-cache--from-cacheflag toalzlibtool generate architectureExportBuiltInCacheensureReferencedPolicyDefinitionsinvocationdefer az.cache = nilfromGetDefinitionsFromAzure(cache retained for AlzLib lifetime)az.cachereads withaz.mu.RLock()inpolicyDefinitionFromCache/policySetDefinitionFromCacheaz.clients.policyClientunderaz.mu.RLock()ingetBuiltInPolicies,getBuiltInPolicySets, andensureReferencedPolicyDefinitionExportBuiltInCachedocstring to include nil PolicyType in the inclusion criteriaOriginal prompt
This pull request was created from Copilot chat.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.