Adding support for version 2023-09-01 by osid29 · Pull Request #23553 · Azure/azure-powershell

osid29 · 2023-11-24T05:56:36Z

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-24T05:56:39Z

️✔️Az.Accounts

️✔️Build

️✔️PowerShell Core - Windows

⚠️Az.PaloAltoNetworks

️✔️Build

️✔️PowerShell Core - Windows

️✔️Breaking Change Check

️✔️PowerShell Core - Windows

⚠️Signature Check

⚠️PowerShell Core - Windows

Type Cmdlet Description Remediation

⚠️ Get-AzPaloAltoNetworksCertificateObjectLocalRulestack Get-AzPaloAltoNetworksCertificateObjectLocalRulestack Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue

⚠️ Get-AzPaloAltoNetworksCertificateObjectLocalRulestack Get-AzPaloAltoNetworksCertificateObjectLocalRulestack changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.

⚠️ Get-AzPaloAltoNetworksFirewall Get-AzPaloAltoNetworksFirewall Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue

⚠️ Get-AzPaloAltoNetworksFirewall Get-AzPaloAltoNetworksFirewall changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.

⚠️ Get-AzPaloAltoNetworksFirewallStatus Get-AzPaloAltoNetworksFirewallStatus Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue

⚠️ Get-AzPaloAltoNetworksFirewallStatus Get-AzPaloAltoNetworksFirewallStatus changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.

⚠️ Get-AzPaloAltoNetworksFqdnListLocalRulestack Get-AzPaloAltoNetworksFqdnListLocalRulestack Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue

⚠️ Get-AzPaloAltoNetworksFqdnListLocalRulestack Get-AzPaloAltoNetworksFqdnListLocalRulestack changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.

⚠️ Get-AzPaloAltoNetworksLocalRule Get-AzPaloAltoNetworksLocalRule Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue

⚠️ Get-AzPaloAltoNetworksLocalRule Get-AzPaloAltoNetworksLocalRule changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.

⚠️ Get-AzPaloAltoNetworksLocalRulestack Get-AzPaloAltoNetworksLocalRulestack Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue

⚠️ Get-AzPaloAltoNetworksLocalRulestack Get-AzPaloAltoNetworksLocalRulestack changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.

⚠️ Get-AzPaloAltoNetworksPrefixListLocalRulestack Get-AzPaloAltoNetworksPrefixListLocalRulestack Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue

⚠️ Get-AzPaloAltoNetworksPrefixListLocalRulestack Get-AzPaloAltoNetworksPrefixListLocalRulestack changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.

⚠️ New-AzPaloAltoNetworksFrontendSettingObject New-AzPaloAltoNetworksFrontendSettingObject changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.

⚠️ New-AzPaloAltoNetworksIPAddressObject New-AzPaloAltoNetworksIPAddressObject changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.

⚠️ New-AzPaloAltoNetworksProfileObject New-AzPaloAltoNetworksProfileObject changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.

⚠️ New-AzPaloAltoNetworksTagInfoObject New-AzPaloAltoNetworksTagInfoObject changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.

️✔️Help Example Check

️✔️PowerShell Core - Windows

️✔️Help File Existence Check

️✔️PowerShell Core - Windows

️✔️File Change Check

️✔️PowerShell Core - Windows

️✔️UX Metadata Check

️✔️PowerShell Core - Windows

⚠️Test

⚠️ - Linux

Type Title Current Coverage Description

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

⚠️ - MacOS

Type Title Current Coverage Description

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

⚠️PowerShell Core - Windows

Type Title Current Coverage Description

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

dolauli · 2023-11-28T06:11:57Z

@osid29 According to the CI failures, there are some issues in your examples. Please fix them.

osamasid · 2023-11-29T09:31:15Z

@dolauli I have updated the PR and the checks seems to pass now. Please help with the merge

dolauli · 2023-12-01T01:03:28Z

src/PaloAltoNetworks/PaloAltoNetworks.Autorest/test/AzPaloAltoNetworks.Tests.ps1

        {
            $config = New-AzPaloAltoNetworksLocalRulestack -Name $env.LocalRulestackName -ResourceGroupName $env.resourceGroup -Location $env.location -Description "testing powershell" -DefaultMode 'NONE'
            $config.Name | Should -Be $env.LocalRulestackName
-        } | Should -Not -Throw


Is special reason that you remove Should -Not -Throw Here.

The build was failing due to missing exception

https://dev.azure.com/azclitools/public/_build/results?buildId=110806&view=logs&j=b17395f6-68a3-5682-0476-d3f6f1043109&t=e50c9eb1-7f19-5aa6-670b-cc46e51b352e

Is special reason that you remove Should -Not -Throw Here.

@osamasid You should not make those changes. The CI should pass without this change.

I reverted the changes and the CI checks are passing now

dolauli · 2023-12-01T01:04:06Z

src/PaloAltoNetworks/PaloAltoNetworks.Autorest/test/Get-AzPaloAltoNetworksFirewall.Tests.ps1

+}
+
+Describe 'Get-AzPaloAltoNetworksFirewall' {
+    It 'List' -skip {


Please add tests for those new added cmdlets.

There are no newly added cmdlets, correct me please if you see any.

It is not about adding new tests, but you disabled all original tests. You will need to rerecord tests after you bump the API versions.

@dolauli I figured out that these tests are not skipped but are duplicates. All cmdlets scenarios are already covered in AzPaloAltoNetworks.Tests.ps1 in create, get, update, remove fashion. I have updated this PR

github-actions · 2023-12-08T01:30:53Z

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.

dolauli · 2023-12-12T02:39:38Z

@osid29 Any comments to my first comment

osid29 · 2023-12-12T04:27:59Z

@osid29 Any comments to my first comment

@dolauli I have addressed both the comments. The CI checks are passing as well

V4 has been set as default, so no need to set it.

dolauli · 2023-12-12T10:17:00Z

src/PaloAltoNetworks/PaloAltoNetworks.Autorest/docs/Az.PaloAltoNetworks.md

@@ -107,12 +107,24 @@ Reset counters
 ### [Save-AzPaloAltoNetworksFirewallLogProfile](Save-AzPaloAltoNetworksFirewallLogProfile.md)
 Log Profile for Firewall



I just noticed you do not add examples and tests for four new cmdlets. These four new cmdlets are new feature of autorest.powershell v4. In v4, if there is no patch API for a resource, we will generate an update-xxx cmdlet by using get->put API to simulate a Patch API.
I would suggest you keep those four cmdlets and add examples and tests for them. If you do not need them, please let me know and I will remove those four cmdlets through a directive.

Hi @dolauli, we don't need below cmdlets now. If we see any adoption, we can update these in future requests. Please remove these.

'Update-AzPaloAltoNetworksFqdnListLocalRulestack',
'Update-AzPaloAltoNetworksLocalRule',
'Update-AzPaloAltoNetworksCertificateObjectLocalRulestack'
'Update-AzPaloAltoNetworksPrefixListLocalRulestack'

Adding support for version 2023-09-01

d61aa6f

Fixed some issues caused by upgrade to v4

bdd9c84

dolauli marked this pull request as draft November 24, 2023 07:26

Fixing failed tests

021c3f7

Removing updates for config objects

5e0487d

dolauli marked this pull request as ready for review November 30, 2023 04:27

dolauli self-assigned this Nov 30, 2023

dolauli requested changes Dec 1, 2023

View reviewed changes

dolauli added the needs-revision label Dec 8, 2023

Removing duplicate skipped tests

5da0eae

Removing throwable exceptions

2839ae3

Update README.md to remove extension configuration

2ff2d37

V4 has been set as default, so no need to set it.

dolauli approved these changes Dec 12, 2023

View reviewed changes

dolauli merged commit 58459af into Azure:generation Dec 12, 2023

azure-powershell-bot mentioned this pull request Dec 12, 2023

Migrate PaloAltoNetworks from generation to main #23715

Closed

1 task

dolauli reviewed Dec 12, 2023

View reviewed changes

		@@ -107,12 +107,24 @@ Reset counters
		### [Save-AzPaloAltoNetworksFirewallLogProfile](Save-AzPaloAltoNetworksFirewallLogProfile.md)
		Log Profile for Firewall

Conversation

osid29 commented Nov 24, 2023

Description

Checklist

Uh oh!

azure-client-tools-bot-prd bot commented Nov 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dolauli commented Nov 28, 2023

Uh oh!

osamasid commented Nov 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dolauli Dec 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 8, 2023

Uh oh!

dolauli commented Dec 12, 2023

Uh oh!

osid29 commented Dec 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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

edited

Loading

dolauli Dec 12, 2023 •

edited

Loading