Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Az.network function1 #18966

Merged
merged 28 commits into from
Aug 15, 2022
Merged

Conversation

kukulkarni1
Copy link

Description

Added custom script for get traffic collector

Checklist

  • 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 the 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 change version of module in pull request

@BethanyZhou
Copy link
Contributor

Could you explain what's your purpose to do this customization so that we can check whether your PR is the correct way to do so?

@BethanyZhou BethanyZhou added the needs-author-feedback More information is needed from author to address the issue. label Jul 20, 2022
@kukulkarni1
Copy link
Author

kukulkarni1 commented Jul 20, 2022

Could you explain what's your purpose to do this customization so that we can check whether your PR is the correct way to do so?
There are 2 things we want to do:

  1. Hide the 2 cmdlets Get-AzNetworkFunctionAzureTrafficCollectorsByResourceGroup and Get-AzNetworkFunctionAzureTrafficCollectorsBySubscription, and add 1 cmdlet Get-AzNetworkFunctionAzureTrafficCollector_List. We want to add 2 parameter sets, 1 is for Get and 1 is for List.
  2. We have made Collector Policy (the child resource) as a Tracked Resource in ARM, so we need to add a new parameter Location to the cmdlet New-AzNetworkFunctionCollectorPolicy. We need to pass this property while creating a new Collector Policy, similar to how we do it for TrafficCollector (the parent resource)

@BethanyZhou
Copy link
Contributor

Hide the 2 cmdlets Get-AzNetworkFunctionAzureTrafficCollectorsByResourceGroup and Get-AzNetworkFunctionAzureTrafficCollectorsBySubscription, and add 1 cmdlet Get-AzNetworkFunctionAzureTrafficCollector_List. We want to add 2 parameter sets, 1 is for Get and 1 is for List.

As you said, Get-AzNetworkFunctionAzureTrafficCollector_List will use Get-AzNetworkFunctionAzureTrafficCollectorsByResourceGroup and Get-AzNetworkFunctionAzureTrafficCollectorsBySubscription. The easiest way to combine them together is changing OperationId of Get-AzNetworkFunctionAzureTrafficCollectorsByResourceGroup and Get-AzNetworkFunctionAzureTrafficCollectorsBySubscription so that they will show as parameter sets of Get-AzNetworkFunctionAzureTrafficCollector directly.

We have made Collector Policy (the child resource) as a Tracked Resource in ARM, so we need to add a new parameter Location to the cmdlet New-AzNetworkFunctionCollectorPolicy. We need to pass this property while creating a new Collector Policy, similar to how we do it for TrafficCollector (the parent resource)

Did this change check into azure-rest-api-spec? Can we do this work just updating the commit id of swagger?

@kukulkarni1
Copy link
Author

kukulkarni1 commented Jul 21, 2022

Hide the 2 cmdlets Get-AzNetworkFunctionAzureTrafficCollectorsByResourceGroup and Get-AzNetworkFunctionAzureTrafficCollectorsBySubscription, and add 1 cmdlet Get-AzNetworkFunctionAzureTrafficCollector_List. We want to add 2 parameter sets, 1 is for Get and 1 is for List.

As you said, Get-AzNetworkFunctionAzureTrafficCollector_List will use Get-AzNetworkFunctionAzureTrafficCollectorsByResourceGroup and Get-AzNetworkFunctionAzureTrafficCollectorsBySubscription. The easiest way to combine them together is changing OperationId of Get-AzNetworkFunctionAzureTrafficCollectorsByResourceGroup and Get-AzNetworkFunctionAzureTrafficCollectorsBySubscription so that they will show as parameter sets of Get-AzNetworkFunctionAzureTrafficCollector directly.

I have made this change in the Readme. I see 2 cmdlets generated, is this correct?
image

Did this change check into azure-rest-api-spec? Can we do this work just updating the commit id of swagger?

We haven't changed our swagger. If the swagger is checked in, will we still need any customization from Powershell side for this cmdlet?
( I am removing the New-AzNetworkFunctionCollectorPolicy cmdlet for now from this PR so that this can get approved. Will create separate PR for that if needed.)

@BethanyZhou
Copy link
Contributor

BethanyZhou commented Jul 22, 2022

I have made this change in the Readme. I see 2 cmdlets generated, is this correct?

Yes, they should be from OperationId AzureTrafficCollectorsBySubscription_List and AzureTrafficCollectorsByResourceGroup_List. Does current Get-AzTrafficCollectors match your requirement now?

We haven't changed our swagger. If the swagger is checked in, will we still need any customization from Powershell side for this cmdlet?
( I am removing the New-AzNetworkFunctionCollectorPolicy cmdlet for now from this PR so that this can get approved. Will create separate PR for that if needed.)

it depends what change has been done in swagger side. Removing module doesn't matter in preview module. Az.NetworkFunction is 0.1.0. all module is in preview status when its version is lower than 1.0.0

@kukulkarni1
Copy link
Author

kukulkarni1 commented Jul 22, 2022

Yes, they should be from OperationId AzureTrafficCollectorsBySubscription_List and AzureTrafficCollectorsByResourceGroup_List. Does current Get-AzTrafficCollectors match your requirement now?

Yes it matches the requirement now. Just 1 question - is there a reason why this isn't part of the automated pipeline in Powershell? If this is a common scenario needed by multiple teams, is there a reason we need to do this manually and it's not automated?

it depends what change has been done in swagger side. Removing module doesn't matter in preview module. Az.NetworkFunction is 0.1.0. all module is in preview status when its version is lower than 1.0.0

Ok, we are trying to make the resource as a tracked resource in swagger. Currently it is a proxy resource. Once this is done, location property should automatically be added as per our understanding - pls correct if wrong.

@BethanyZhou
Copy link
Contributor

Just 1 question - is there a reason why this isn't part of the automated pipeline in Powershell? If this is a common scenario needed by multiple teams, is there a reason we need to do this manually and it's not automated?

As we assume that an operation id should follow pattern {Noun}_{Verb}, so our generator recognizes AzureTrafficCollectorsBySubscription as a resource by operation id AzureTrafficCollectorsBySubscription_List. Many teams follow the pattern above to name operation id. If the team does not follow the convention, we need to do this manually.

Ok, we are trying to make the resource as a tracked resource in swagger. Currently it is a proxy resource. Once this is done, location property should automatically be added as per our understanding - pls correct if wrong.

I guess you can modify swagger by directive to make the resource as a tracked resource by far to reach your goal.

    "CollectorPolicy": {
      "type": "object",
      "properties": {
        "properties": {
          "x-ms-client-flatten": true,
          "$ref": "#/definitions/CollectorPolicyPropertiesFormat",
          "description": "Properties of the Collector Policy."
        },
        "etag": {
          "type": "string",
          "readOnly": true,
          "description": "A unique read-only string that changes whenever the resource is updated."
        },
        "systemData": {
          "allOf": [
            {
              "$ref": "#/definitions/SystemData"
            }
          ],
          "description": "Metadata pertaining to creation and last modification of the resource.",
          "readOnly": true
        }
      },
      "allOf": [
        {
          "$ref": "#/definitions/ProxyResource" --------------------> TrackedResource
        }
      ],
      "description": "Collector policy resource."
    },

@kukulkarni1
Copy link
Author

Just 1 question - is there a reason why this isn't part of the automated pipeline in Powershell? If this is a common scenario needed by multiple teams, is there a reason we need to do this manually and it's not automated?

As we assume that an operation id should follow pattern {Noun}_{Verb}, so our generator recognizes AzureTrafficCollectorsBySubscription as a resource by operation id AzureTrafficCollectorsBySubscription_List. Many teams follow the pattern above to name operation id. If the team does not follow the convention, we need to do this manually.

Ok, we are trying to make the resource as a tracked resource in swagger. Currently it is a proxy resource. Once this is done, location property should automatically be added as per our understanding - pls correct if wrong.

I guess you can modify swagger by directive to make the resource as a tracked resource by far to reach your goal.

    "CollectorPolicy": {
      "type": "object",
      "properties": {
        "properties": {
          "x-ms-client-flatten": true,
          "$ref": "#/definitions/CollectorPolicyPropertiesFormat",
          "description": "Properties of the Collector Policy."
        },
        "etag": {
          "type": "string",
          "readOnly": true,
          "description": "A unique read-only string that changes whenever the resource is updated."
        },
        "systemData": {
          "allOf": [
            {
              "$ref": "#/definitions/SystemData"
            }
          ],
          "description": "Metadata pertaining to creation and last modification of the resource.",
          "readOnly": true
        }
      },
      "allOf": [
        {
          "$ref": "#/definitions/ProxyResource" --------------------> TrackedResource
        }
      ],
      "description": "Collector policy resource."
    },

Thanks - we have made the swagger change and PR is under review: Azure/azure-rest-api-specs#19888

@BethanyZhou
Copy link
Contributor

That's good. We have two options now.

  • Wait for swagger change finished. And then update the commid id in README.md.
  • Or modify swagger by directive of autorest.powershell.
    Which one do you prefer

@BethanyZhou
Copy link
Contributor

like @isra-fel said, the best way is marking Location in TrackedResource as required. Is it doable?

@kukulkarni1
Copy link
Author

like @isra-fel said, the best way is marking Location in TrackedResource as required. Is it doable?

We have time constraints since we have our public preview this month. Changing swagger again would require us to go through more review processes and would need additional testing. Is it possible to keep the customization on the powershell side? Thanks.

@BethanyZhou
Copy link
Contributor

Looks good to me. No update-* cmdlet is expected, is it?

@kukulkarni1
Copy link
Author

Looks good to me. No update-* cmdlet is expected, is it?

Actually, the set cmdlets that we had included in the earlier commit were supposed to be the update cmdlets. I have added them again, and named them as update and also made the location property mandatory in them. Those cmdlets are used for updating the traffic collector and collector policy.

@kukulkarni1
Copy link
Author

Hi @BethanyZhou @isra-fel Is this good to be merged?

@isra-fel
Copy link
Member

@kukulkarni1 Hi, can you write down a list of changes that we can put into change log?

@isra-fel isra-fel mentioned this pull request Aug 12, 2022
1 task
Copy link
Member

@isra-fel isra-fel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix these issues in examples. And don't forget to run ./build-module.ps1 to refresh doc/ folder.

  "Module","Cmdlet","Example","Line","RuleName","ProblemId","Severity","Description","Extent","Remediation"
  "NetworkFunction","Get-AzNetworkFunctionCollectorPolicy","1","1","Invalid_Parameter_Name","5011","1","Get-AzNetworkFunctionCollectorPolicy -resourcegroup is not a valid parameter name.","-resourcegroup","Check validity of the parameter -resourcegroup."
  "NetworkFunction","Get-AzNetworkFunctionTrafficCollector","2","1","Invalid_Parameter_Name","5011","1","Get-AzNetworkFunctionTrafficCollector -ResourceGroup is not a valid parameter name.","-ResourceGroup","Check validity of the parameter -ResourceGroup."
  "NetworkFunction","Get-AzNetworkFunctionTrafficCollector","3","1","Invalid_Parameter_Name","5011","1","Get-AzNetworkFunctionTrafficCollector -ResourceGroup is not a valid parameter name.","-ResourceGroup","Check validity of the parameter -ResourceGroup."
  "NetworkFunction","New-AzNetworkFunctionCollectorPolicy","1","1","Unassigned_Parameter","5013","1","New-AzNetworkFunctionCollectorPolicy -azuretrafficcollectorname must be assigned with a value.","-azuretrafficcollectorname","Assign value for the parameter -azuretrafficcollectorname."
  "NetworkFunction","New-AzNetworkFunctionCollectorPolicy","1","1","Invalid_Parameter_Name","5011","1","New-AzNetworkFunctionCollectorPolicy -atc is not a valid parameter name.","-atc","Check validity of the parameter -atc."
  "NetworkFunction","New-AzNetworkFunctionCollectorPolicy","1","1","Invalid_Parameter_Name","5011","1","New-AzNetworkFunctionCollectorPolicy -resourcegroup is not a valid parameter name.","-resourcegroup","Check validity of the parameter -resourcegroup."
  "NetworkFunction","New-AzNetworkFunctionTrafficCollector","1","1","Invalid_Parameter_Name","5011","1","New-AzNetworkFunctionTrafficCollector -resourcegroup is not a valid parameter name.","-resourcegroup","Check validity of the parameter -resourcegroup."
  "NetworkFunction","Remove-AzNetworkFunctionCollectorPolicy","1","1","Invalid_Parameter_Name","5011","1","Remove-AzNetworkFunctionCollectorPolicy -resourcegroup is not a valid parameter name.","-resourcegroup","Check validity of the parameter -resourcegroup."
  "NetworkFunction","Remove-AzNetworkFunctionTrafficCollector","1","1","Invalid_Parameter_Name","5011","1","Remove-AzNetworkFunctionTrafficCollector -resourcegroup is not a valid parameter name.","-resourcegroup","Check validity of the parameter -resourcegroup."
  "NetworkFunction","Update-AzNetworkFunctionCollectorPolicy","1","1","Unassigned_Parameter","5013","1","Update-AzNetworkFunctionCollectorPolicy -azuretrafficcollectorname must be assigned with a value.","-azuretrafficcollectorname","Assign value for the parameter -azuretrafficcollectorname."
  "NetworkFunction","Update-AzNetworkFunctionCollectorPolicy","1","1","Invalid_Parameter_Name","5011","1","Update-AzNetworkFunctionCollectorPolicy -atc is not a valid parameter name.","-atc","Check validity of the parameter -atc."
  "NetworkFunction","Update-AzNetworkFunctionCollectorPolicy","1","1","Invalid_Parameter_Name","5011","1","Update-AzNetworkFunctionCollectorPolicy -resourcegroup is not a valid parameter name.","-resourcegroup","Check validity of the parameter -resourcegroup."
  "NetworkFunction","Update-AzNetworkFunctionTrafficCollector","1","1","Invalid_Parameter_Name","5011","1","Update-AzNetworkFunctionTrafficCollector -resourcegroup is not a valid parameter name.","-resourcegroup","Check validity of the parameter -resourcegroup."
  "NetworkFunction","Update-AzNetworkFunctionTrafficCollectorTag","1","1","Invalid_Parameter_Name","5011","1","Update-AzNetworkFunctionTrafficCollectorTag -resourcegroup is not a valid parameter name.","-resourcegroup","Check validity of the parameter -resourcegroup."

@kukulkarni1
Copy link
Author

@kukulkarni1 Hi, can you write down a list of changes that we can put into change log?

  1. Made Collector policy a tracked resource (added location property to create and update cmdlet and made it mandatory)
  2. Change prefix of cmdlets from AzureTrafficCollector to TrafficCollector
  3. Change operation id of list cmdlets to remove the cmdlets Get-AzNetworkFunctionAzureTrafficCollectorsByResourceGroup and Get-AzNetworkFunctionAzureTrafficCollectorsBySubscription and call them internally based on parameters provided to the cmdlet Get-AzNetworkFunctionAzureTrafficCollector_List.

@kukulkarni1
Copy link
Author

kukulkarni1 commented Aug 12, 2022

Please fix these issues in examples. And don't forget to run ./build-module.ps1 to refresh doc/ folder.

  "Module","Cmdlet","Example","Line","RuleName","ProblemId","Severity","Description","Extent","Remediation"
  "NetworkFunction","Get-AzNetworkFunctionCollectorPolicy","1","1","Invalid_Parameter_Name","5011","1","Get-AzNetworkFunctionCollectorPolicy -resourcegroup is not a valid parameter name.","-resourcegroup","Check validity of the parameter -resourcegroup."
  "NetworkFunction","Get-AzNetworkFunctionTrafficCollector","2","1","Invalid_Parameter_Name","5011","1","Get-AzNetworkFunctionTrafficCollector -ResourceGroup is not a valid parameter name.","-ResourceGroup","Check validity of the parameter -ResourceGroup."
  "NetworkFunction","Get-AzNetworkFunctionTrafficCollector","3","1","Invalid_Parameter_Name","5011","1","Get-AzNetworkFunctionTrafficCollector -ResourceGroup is not a valid parameter name.","-ResourceGroup","Check validity of the parameter -ResourceGroup."
  "NetworkFunction","New-AzNetworkFunctionCollectorPolicy","1","1","Unassigned_Parameter","5013","1","New-AzNetworkFunctionCollectorPolicy -azuretrafficcollectorname must be assigned with a value.","-azuretrafficcollectorname","Assign value for the parameter -azuretrafficcollectorname."
  "NetworkFunction","New-AzNetworkFunctionCollectorPolicy","1","1","Invalid_Parameter_Name","5011","1","New-AzNetworkFunctionCollectorPolicy -atc is not a valid parameter name.","-atc","Check validity of the parameter -atc."
  "NetworkFunction","New-AzNetworkFunctionCollectorPolicy","1","1","Invalid_Parameter_Name","5011","1","New-AzNetworkFunctionCollectorPolicy -resourcegroup is not a valid parameter name.","-resourcegroup","Check validity of the parameter -resourcegroup."
  "NetworkFunction","New-AzNetworkFunctionTrafficCollector","1","1","Invalid_Parameter_Name","5011","1","New-AzNetworkFunctionTrafficCollector -resourcegroup is not a valid parameter name.","-resourcegroup","Check validity of the parameter -resourcegroup."
  "NetworkFunction","Remove-AzNetworkFunctionCollectorPolicy","1","1","Invalid_Parameter_Name","5011","1","Remove-AzNetworkFunctionCollectorPolicy -resourcegroup is not a valid parameter name.","-resourcegroup","Check validity of the parameter -resourcegroup."
  "NetworkFunction","Remove-AzNetworkFunctionTrafficCollector","1","1","Invalid_Parameter_Name","5011","1","Remove-AzNetworkFunctionTrafficCollector -resourcegroup is not a valid parameter name.","-resourcegroup","Check validity of the parameter -resourcegroup."
  "NetworkFunction","Update-AzNetworkFunctionCollectorPolicy","1","1","Unassigned_Parameter","5013","1","Update-AzNetworkFunctionCollectorPolicy -azuretrafficcollectorname must be assigned with a value.","-azuretrafficcollectorname","Assign value for the parameter -azuretrafficcollectorname."
  "NetworkFunction","Update-AzNetworkFunctionCollectorPolicy","1","1","Invalid_Parameter_Name","5011","1","Update-AzNetworkFunctionCollectorPolicy -atc is not a valid parameter name.","-atc","Check validity of the parameter -atc."
  "NetworkFunction","Update-AzNetworkFunctionCollectorPolicy","1","1","Invalid_Parameter_Name","5011","1","Update-AzNetworkFunctionCollectorPolicy -resourcegroup is not a valid parameter name.","-resourcegroup","Check validity of the parameter -resourcegroup."
  "NetworkFunction","Update-AzNetworkFunctionTrafficCollector","1","1","Invalid_Parameter_Name","5011","1","Update-AzNetworkFunctionTrafficCollector -resourcegroup is not a valid parameter name.","-resourcegroup","Check validity of the parameter -resourcegroup."
  "NetworkFunction","Update-AzNetworkFunctionTrafficCollectorTag","1","1","Invalid_Parameter_Name","5011","1","Update-AzNetworkFunctionTrafficCollectorTag -resourcegroup is not a valid parameter name.","-resourcegroup","Check validity of the parameter -resourcegroup."

I have renamed the resourcegroup parameter to resourcegroupname in the examples. also pushed the latest doc folder after running ./build-modules.ps1

@isra-fel isra-fel merged commit 9a98bd5 into Azure:generation Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants