[ChangeSafety] Phase 1: AcquirePolicyToken Support#28711
[ChangeSafety] Phase 1: AcquirePolicyToken Support#28711
Conversation
| Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
There was a problem hiding this comment.
Pull Request Overview
Introduces Phase 1 support for Azure Policy Change Safety by adding a configurable EnablePolicyToken flag and plumbing dynamic parameters (e.g. -AcquirePolicyToken) through selected cmdlets. Key changes include adding the EnablePolicyToken config and test, refactoring dynamic parameter handling in storage file content cmdlets, and replacing prior package references with extensive project references to azure-powershell-common to surface new shared functionality.
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/Common.Netcore.Dependencies.targets | Comments out prior package references (now relying on project references in individual csproj files). |
| tools/AzDev/src/AzDev.csproj | Downgrades select packages to .NET 8-compatible versions. |
| src/shared/ConfigKeys.cs | Adds new EnablePolicyToken config key. |
| src/Storage/Storage/Storage.csproj | Adds project references to common libraries for policy token support. |
| src/Storage/Storage/File/Cmdlet/SetAzureStorageFileContent.cs | Refactors dynamic parameter merging to include new base parameters. |
| src/Storage/Storage/File/Cmdlet/GetAzureStorageFileContent.cs | Same refactor for dynamic parameter merging on get cmdlet. |
| src/Storage/Storage.common/Storage.common.csproj | Adds project references to shared modules. |
| src/Storage/Storage.Management/Storage.Management.csproj | Adds project references to shared modules. |
| src/Storage/Storage.Management.Sdk/Storage.Management.Sdk.csproj | Adds project references to shared modules. |
| src/Resources/Tags/Tags.csproj | Adds project references to shared modules. |
| src/Resources/ResourceManager/ResourceManager.csproj | Adds project references to shared modules. |
| src/Resources/Authorization.Management.Sdk/Authorization.Management.Sdk.csproj | Adds project references to shared modules. |
| src/Accounts/Authentication/Config/Definitions/EnablePolicyTokenConfig.cs | Introduces the EnablePolicyToken config definition. |
| src/Accounts/Authentication/Config/ConfigInitializer.cs | Registers the new config. |
| src/Accounts/Authentication/Authentication.csproj | Adds project references for shared functionality access. |
| src/Accounts/AssemblyLoading/AssemblyLoading.csproj | Adds Common project reference. |
| src/Accounts/Accounts.sln | Adds solution entries for referenced common projects. |
| src/Accounts/Accounts.Test/UnitTest/EnablePolicyTokenConfigTest.cs | Adds unit test validating config defaults/key. |
| documentation/change-safety.md | Adds feature documentation (Phase 1) for change safety tokens. |
| // Non-Windows: just pass through whatever base produced (could be null or already a dictionary including AcquirePolicyToken) | ||
| return baseResult; | ||
| } | ||
| private WindowsOnlyParameters context; |
There was a problem hiding this comment.
The previous implementation relied on returning the WindowsOnlyParameters object so parameter binding populated context.PreserveSMBAttribute; now a RuntimeDefinedParameterDictionary is returned and the bound value will reside in dict["PreserveSMBAttribute"].Value, leaving context.PreserveSMBAttribute at its default and breaking downstream logic that reads context. Either (a) set up a post-binding synchronization (e.g. override BeginProcessing to copy from the runtime defined parameter value into context) or (b) preserve prior behavior by returning an IDynamicParameters object that encapsulates both base and Windows-only parameters. For a minimal fix, after adding rd, attach a handler or in processing retrieve value from dict instead of context.
| private WindowsOnlyParameters context; | |
| private WindowsOnlyParameters context; | |
| protected override void BeginProcessing() | |
| { | |
| base.BeginProcessing(); | |
| // Synchronize PreserveSMBAttribute from bound parameters to context | |
| if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && context != null) | |
| { | |
| object value; | |
| if (this.MyInvocation.BoundParameters.TryGetValue("PreserveSMBAttribute", out value)) | |
| { | |
| context.PreserveSMBAttribute = value is bool b ? b : false; | |
| } | |
| } | |
| } |
| public override object GetDynamicParameters() | ||
| { | ||
| var baseResult = base.GetDynamicParameters(); | ||
| var dict = baseResult as RuntimeDefinedParameterDictionary; | ||
|
|
||
| if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
| { | ||
| context = new WindowsOnlyParameters(); | ||
| return context; | ||
|
|
||
| if (dict == null) | ||
| { | ||
| dict = new RuntimeDefinedParameterDictionary(); | ||
| } | ||
|
|
||
| var winParamProp = typeof(WindowsOnlyParameters).GetProperty("PreserveSMBAttribute"); | ||
| if (winParamProp != null && !dict.ContainsKey(winParamProp.Name)) | ||
| { | ||
| var attrs = winParamProp.GetCustomAttributes(true).OfType<Attribute>().ToArray(); | ||
| var attrCollection = new Collection<Attribute>(attrs.ToList()); | ||
| var rd = new RuntimeDefinedParameter(winParamProp.Name, winParamProp.PropertyType, attrCollection); | ||
| dict.Add(winParamProp.Name, rd); | ||
| } | ||
|
|
||
| return dict; | ||
| } | ||
| else return null; | ||
|
|
||
| return baseResult; | ||
| } |
There was a problem hiding this comment.
Same binding issue as in SetAzureStorageFileContent: returning a dictionary prevents automatic population of context.PreserveSMBAttribute, so any later usage of context will see the default value. Apply the same fix: copy dict["PreserveSMBAttribute"].Value into context.PreserveSMBAttribute before it's consumed, or revert to returning an object that exposes the property directly for binding.
|
|
||
| public object GetDynamicParameters() | ||
| // Override to cooperatively merge base dynamic parameters (e.g. AcquirePolicyToken) with existing Windows-only ones. | ||
| public override object GetDynamicParameters() |
There was a problem hiding this comment.
[nitpick] The dynamic parameter merging logic introduced here is duplicated almost verbatim in GetAzureStorageFileContent; consider extracting a shared helper (e.g. a protected method in a common base class) to reduce duplication and ensure future changes (additional Windows-only parameters or new base dynamic parameters) stay consistent.
|
|
||
| // Non-Windows: just pass through whatever base produced (could be null or already a dictionary including AcquirePolicyToken) | ||
| return baseResult; | ||
| } |
There was a problem hiding this comment.
[nitpick] The dynamic parameter merging logic introduced here is duplicated almost verbatim in GetAzureStorageFileContent; consider extracting a shared helper (e.g. a protected method in a common base class) to reduce duplication and ensure future changes (additional Windows-only parameters or new base dynamic parameters) stay consistent.
| } | ||
| public object GetDynamicParameters() | ||
| // Override to merge base dynamic parameters (e.g. AcquirePolicyToken) with existing Windows-only parameter. | ||
| public override object GetDynamicParameters() |
There was a problem hiding this comment.
[nitpick] Duplicate dynamic parameter merge logic; refactor into a reusable method or utility to avoid divergence and simplify future parameter additions.
| else return null; | ||
|
|
||
| return baseResult; | ||
| } |
There was a problem hiding this comment.
[nitpick] Duplicate dynamic parameter merge logic; refactor into a reusable method or utility to avoid divergence and simplify future parameter additions.
| | Phase | Scope | Visible Parameter(s) | Notes | | ||
| |-------|-------|----------------------|-------| | ||
| | Phase 1 (current) | Token acquisition only | `-AcquirePolicyToken` | `-ChangeReference` is suppressed/hidden. | | ||
| | Phase 2 (planned) | Adds change reference association | `-AcquirePolicyToken`, `-ChangeReference` | `-ChangeReference` implies token acquisition. | |
There was a problem hiding this comment.
The tables use a double leading pipe (||) which breaks standard Markdown table rendering; remove the extra leading pipe so each row begins with a single | to ensure proper formatting.
| | Parameter | Type | Phase | Purpose | | ||
| |----------|------|-------|---------| | ||
| | `-AcquirePolicyToken` | Switch | 1+ | Force acquisition of a change-safety policy token for the operation. | | ||
| | `-ChangeReference <string>` | String | 2 | (Planned) Associates the operation with an external change record (e.g., a ChangeState resource ID); implies token acquisition. Not yet surfaced. | |
There was a problem hiding this comment.
The tables use a double leading pipe (||) which breaks standard Markdown table rendering; remove the extra leading pipe so each row begins with a single | to ensure proper formatting.
| | Symptom | Possible Cause | Mitigation | | ||
| |--------|----------------|-----------| | ||
| | Parameters not visible | Feature flag not enabled or common library not updated | Run `Update-AzConfig -EnablePolicyToken $true` and ensure you have a version that includes the handler. | |
There was a problem hiding this comment.
The tables use a double leading pipe (||) which breaks standard Markdown table rendering; remove the extra leading pipe so each row begins with a single | to ensure proper formatting.
|
Related to this change: Azure/azure-powershell-common#449 TODO:
|
1 participant
Description
Mandatory Checklist
Please choose the target release of Azure PowerShell. (⚠️ Target release is a different concept from API readiness. Please click below links for details.)
Check this box to confirm: I have read the Submitting Changes section of
CONTRIBUTING.mdand reviewed the following information:ChangeLog.mdfile(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.## Upcoming Releaseheader in the past tense.ChangeLog.mdif no new release is required, such as fixing test case only.