Update Automation Module cmdlets to support Powershell7.2 #23482

sushil490023 · 2023-11-20T05:10:24Z

Description

Checklist

Check this box to confirm: I have read the Submitting Changes section of CONTRIBUTING.md and reviewed the following information:

SHOULD select appropriate branch. Cmdlets from Autorest.PowerShell should go to generation branch.
SHOULD make the title of PR clear and informative, and in the present imperative tense.
SHOULD update ChangeLog.md file(s) appropriately
- For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
- A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header in the past tense. Add changelog in description section if PR goes into generation branch.
- Should not change ChangeLog.md if no new release is required, such as fixing test case only.
SHOULD have approved design review for the changes in this repository (Microsoft internal only) with following situations
- Create new module from scratch
- Create new resource types which are not easy to conform to Azure PowerShell Design Guidelines
- Create new resource type which name doesn't use module name as prefix
- Have design question before implementation
SHOULD regenerate markdown help files if there is cmdlet API change. Instruction
SHOULD have proper test coverage for changes in pull request.
SHOULD NOT introduce breaking changes in Az minor release except preview version.
SHOULD NOT adjust version of module manually in pull request

azure-client-tools-bot-prd · 2023-11-20T05:10:28Z

️✔️Az.Accounts

️✔️Build

️✔️PowerShell Core - Windows

️✔️Windows PowerShell - Windows

⚠️Az.Automation

️✔️Build

️✔️PowerShell Core - Windows

️✔️Windows PowerShell - Windows

️✔️Breaking Change Check

️✔️PowerShell Core - Windows

️✔️Windows PowerShell - Windows

️✔️Signature Check

️✔️PowerShell Core - Windows

️✔️Windows PowerShell - Windows

️✔️Help Example Check

️✔️PowerShell Core - Windows

️✔️Windows PowerShell - Windows

️✔️Help File Existence Check

️✔️PowerShell Core - Windows

️✔️Windows PowerShell - Windows

️✔️File Change Check

️✔️PowerShell Core - Windows

️✔️Windows PowerShell - Windows

️✔️UX Metadata Check

️✔️PowerShell Core - Windows

️✔️Windows PowerShell - Windows

️✔️Generated Sdk Check

️✔️PowerShell Core - Windows

️✔️Windows PowerShell - Windows

⚠️Test

⚠️ - Linux

Type Title Current Coverage Description

⚠️ Test Coverage Less Than 50% 38.00 % Test coverage for the module cannot be lower than 50%.

⚠️ - MacOS

Type Title Current Coverage Description

⚠️ Test Coverage Less Than 50% 38.00% Test coverage for the module cannot be lower than 50%.

⚠️PowerShell Core - Windows

Type Title Current Coverage Description

⚠️ Test Coverage Less Than 50% 38.00% Test coverage for the module cannot be lower than 50%.

⚠️Windows PowerShell - Windows

Type Title Current Coverage Description

⚠️ Test Coverage Less Than 50% 38.00% Test coverage for the module cannot be lower than 50%.

NoriZC · 2023-11-20T07:58:37Z

Hi @sushil490023 To pass the CI pipelines:

Please re-record the failed test cases.
Do you think the Position = 4 for RuntimeVersion is necessary? If not, please remove it because a parameter with a position larger than four is discouraged.

src/Automation/Automation/ChangeLog.md

src/Automation/Automation/Cmdlet/GetAzureAutomationModule.cs

src/Automation/Automation/Cmdlet/NewAzureAutomationModule.cs

NoriZC · 2023-11-20T08:36:34Z

src/Automation/Automation/Common/AutomationPSClient.cs

+        public Module GetPowerShell72Module(string resourceGroupName, string automationAccountName, string name)
+        {
+            try
+            {
+                var module =
+                    this.automationManagementClient.Module.Get(resourceGroupName, automationAccountName, name);
+                return new Module(resourceGroupName, automationAccountName, module);
+            }
+            catch (ErrorResponseException cloudException)
+            {
+                if (cloudException.Response.StatusCode == System.Net.HttpStatusCode.NotFound)
+                {
+                    throw new ResourceNotFoundException(typeof(Module),
+                        string.Format(CultureInfo.CurrentCulture, Resources.ModuleNotFound, name));
+                }
+
+                throw;
+            }
+        }


I didn't see any difference between this function and L352-L370. Could you explain why a new set of PowerShell72 is needed?

I have Updated the function. Thanks for pointing it

src/Automation/Automation/Common/AutomationPSClient.cs

src/Automation/Automation/Cmdlet/GetAzureAutomationModule.cs

github-actions · 2023-11-21T03:41:34Z

This PR was labeled "needs-revision" because it has unresolved review comments or CI failures.
Please resolve all open review comments and make sure all CI checks are green. Refer to our guide to troubleshoot common CI failures.

src/Automation/Automation/Common/AutomationPSClient.cs

NoriZC · 2023-11-22T03:27:20Z

Hi @sushil490023

The affetced test cases are not re-recorded. Please record them ans make sure they can work
As I commented above, you still have Module which haven't been updated as PowerShell72Module. Currently we cannot determine you code correctness by reviewing. Could you design a set of e2e tests to cover the senario?
The difference between function Create() and CreatePowerShell72Module() is small, which means there are lots of unnecessary overlaps. Could you refine the code to make it simpler? (For example, can we add a optional parameter into the create() to flag PowerShell 7.2?)

sushil490023 · 2023-11-22T14:13:17Z

Hi @NoriZC ,

I have recorded the test cases impacted by swagger changes in Azure/azure-rest-api-specs@9807153. The test cases for UpdateManagementTests and UpdateDynamicGroupPrePostTests are not recorded as there have been no changes in the SDK, apart from the API version change.

Recording these end-to-end (E2E) test cases requires setting up resources, which will take some time. We are actively working on adding these test cases with a subsequent pull request (PR) along with Abhimanyu Varma and Sanjeev Kumar.

The existing set of E2E test cases currently verifies that the module is created and updated correctly. However, the response object of the cmdlets (i.e., Module) does not have any distinct parameter to distinguish whether it's PowerShell 5 or 7. I have added an explicit assertion to check that no PowerShell 7.2 modules are created or imported in PowerShell 5.1 operations, and vice versa for PowerShell 5.1.
Further simplification in AutomationPSClient.cs would impact the readability of the code with the addition of if-else statements, as for most lines of code, "Module" needs to be replaced with "PowerShell72Module."

Thanks

NoriZC · 2023-11-23T13:17:32Z

Hi @sushil490023

Big thanks that you can understand our practice of keeping the test cases running for code quality. For the currently unrecorded and failed cases, please
- confirm that the behavior is the same as before after upgrading the API version.
- give an ETA of recording.
After that you can skip them by adding [Fact(Skip = "<the reason why this is temporarily skipped>")]. Here is an example.
The e2e tests look good.
I understand your concern. But as shown below, the two method has little differences. Except the line I flagged, other code are duplicated unnecessarily. We should reduce the duplication of code. If you think the if-else is unnecessarily introduced as I suggested, we can discuss a better one. But at least the current duplication is unexpected as well.

public Module GetModule(string resourceGroupName, string automationAccountName, string name)
        {
            try
            {
                var module =
+                    this.automationManagementClient.Module.Get(resourceGroupName, automationAccountName, name);
                return new Module(resourceGroupName, automationAccountName, module);
            }
            catch (ErrorResponseException cloudException)
            {
                if (cloudException.Response.StatusCode == System.Net.HttpStatusCode.NotFound)
                {
                    throw new ResourceNotFoundException(typeof(Module),
                        string.Format(CultureInfo.CurrentCulture, Resources.ModuleNotFound, name));
                }

                throw;
            }
        }

public Module GetPowerShell72Module(string resourceGroupName, string automationAccountName, string name)
        {
            try
            {
                var module =
+                   this.automationManagementClient.PowerShell72Module.Get(resourceGroupName, automationAccountName, name);
                return new Module(resourceGroupName, automationAccountName, module);
            }
            catch (ErrorResponseException cloudException)
            {
                if (cloudException.Response.StatusCode == System.Net.HttpStatusCode.NotFound)
                {
                    throw new ResourceNotFoundException(typeof(Module),
                        string.Format(CultureInfo.CurrentCulture, Resources.ModuleNotFound, name));
                }

                throw;
            }
        }

Please follow the second point I mentioned in this comment to check the Position in parameter. I will locate one case and raise a suggestion for you. We are not encouraging position parameter any more now.

Hi @sushil490023 To pass the CI pipelines:

Please re-record the failed test cases.

Do you think the Position = 4 for RuntimeVersion is necessary? If not, please remove it because a parameter with a position larger than four is discouraged.

src/Automation/Automation/Cmdlet/GetAzureAutomationModule.cs

sushil490023 · 2023-11-24T11:29:01Z

Hi @NoriZC

I can confirm that no changes in skipped test cases apart from API versions changes and ETA for recording test case is 12/5.
I have refactored the code now to use overlapping code.
Removed positional parameter from all effected cmdlets.

Thanks

microsoft-github-policy-service bot added the customer-reported label Nov 20, 2023

Update Automation Module cmdlets to support Powershell7.2

2fd4ee2

sushil490023 force-pushed the PS72SDK branch from adf50ad to 2fd4ee2 Compare November 20, 2023 05:24

isra-fel added the needs-review label Nov 20, 2023

isra-fel assigned NoriZC Nov 20, 2023

isra-fel added this to the Az 11.1.0 (December 2023, 2023-12-05) milestone Nov 20, 2023

NoriZC requested changes Nov 20, 2023

View reviewed changes

NoriZC added needs-revision and removed needs-review labels Nov 21, 2023

Fixed Errors

c0527db

NoriZC reviewed Nov 22, 2023

View reviewed changes

src/Automation/Automation/Common/AutomationPSClient.cs Outdated Show resolved Hide resolved

Fix Test Cases

9f0e1a3

sushil490023 force-pushed the PS72SDK branch from 7e0bb09 to 9f0e1a3 Compare November 22, 2023 13:53

Updated Help files

fcbc7a0

NoriZC reviewed Nov 23, 2023

View reviewed changes

src/Automation/Automation/Cmdlet/GetAzureAutomationModule.cs Outdated Show resolved Hide resolved

Updated Help files

dc040ca

NoriZC added 2 commits November 24, 2023 22:44

Update UpdateManagementTests.cs

8df2be2

Update UpdateDynamicGroupPrePostTests.cs

c3a4781

NoriZC approved these changes Nov 26, 2023

View reviewed changes

NoriZC merged commit 3981d9d into Azure:main Nov 26, 2023
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Automation Module cmdlets to support Powershell7.2 #23482

Update Automation Module cmdlets to support Powershell7.2 #23482

sushil490023 commented Nov 20, 2023

azure-client-tools-bot-prd bot commented Nov 20, 2023 •

edited

Loading

NoriZC commented Nov 20, 2023 •

edited

Loading

NoriZC Nov 20, 2023

sushil490023 Nov 21, 2023

github-actions bot commented Nov 21, 2023

NoriZC commented Nov 22, 2023

sushil490023 commented Nov 22, 2023

NoriZC commented Nov 23, 2023

sushil490023 commented Nov 24, 2023

Update Automation Module cmdlets to support Powershell7.2 #23482

Update Automation Module cmdlets to support Powershell7.2 #23482

Conversation

sushil490023 commented Nov 20, 2023

Description

Checklist

azure-client-tools-bot-prd bot commented Nov 20, 2023 • edited Loading

NoriZC commented Nov 20, 2023 • edited Loading

NoriZC Nov 20, 2023

Choose a reason for hiding this comment

sushil490023 Nov 21, 2023

Choose a reason for hiding this comment

github-actions bot commented Nov 21, 2023

NoriZC commented Nov 22, 2023

sushil490023 commented Nov 22, 2023

NoriZC commented Nov 23, 2023

sushil490023 commented Nov 24, 2023

azure-client-tools-bot-prd bot commented Nov 20, 2023 •

edited

Loading

NoriZC commented Nov 20, 2023 •

edited

Loading