Az.network function1

[kukulkarni1]

Description

Added custom script for get traffic collector

Checklist

• [x] 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 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]

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?

[kukulkarni1]

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]

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.

• OperationId AzureTrafficCollectorsBySubscription_List ->AzureTrafficCollectors_ListBySubscription and
• OperationId AzureTrafficCollectorsByResourceGroup_List -> AzureTrafficCollectors_ListByResourceGroup See example: https://cs.github.com/Azure/azure-powershell/blob/086ce98d157d5fe3513d3a4c6dc1ff9412fd06cf/src/GuestConfiguration/README.md#L57

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]

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.

• OperationId AzureTrafficCollectorsBySubscription_List ->AzureTrafficCollectors_ListBySubscription and
• OperationId AzureTrafficCollectorsByResourceGroup_List -> AzureTrafficCollectors_ListByResourceGroup See example: https://cs.github.com/Azure/azure-powershell/blob/086ce98d157d5fe3513d3a4c6dc1ff9412fd06cf/src/GuestConfiguration/README.md#L57

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

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]

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]

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]

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]

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: https://github.com/Azure/azure-rest-api-specs/pull/19888

[BethanyZhou]

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

[kukulkarni1]

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

We can wait for the commit id.

[BethanyZhou]

Let me convert current PR as draft and please mark it as ready when the commit id comes.

[kukulkarni1]

Let me convert current PR as draft and please mark it as ready when the commit id comes.

I have updated the commit id in the readme - looks good now.

[BethanyZhou]

So does New-AzNetworkFunctionCollectorPolicy include Location now? If so, please run autorest and ./build-module.ps and commit all changes to your remote branch

[kukulkarni1]

So does New-AzNetworkFunctionCollectorPolicy include Location now? If so, please run autorest and ./build-module.ps and commit all changes to your remote branch

Yes New-AzNetworkFunctionCollectorPolicy has location now. We have followed PS guidelines and checked in all needed files. 1 question - How do we make the location property as mandatory? What is the right way to do this?

[BethanyZhou]

Per my knowledge, the way to make parameter mandatory is via customization:

1. Hide New-AzNetworkFunctionCollectorPolicy by directive
2. Copy New-AzNetworkFunctionCollectorPolicy.ps1 from internal folder to custom folder
3. Add attribute mandatory = true for location

@isra-fel Do we support other ways?

[BethanyZhou]

We have followed PS guidelines and checked in all needed files

I haven't found the changes for syntax of docs/New-AzNetworkFunctionCollectorPolicy.md and docs/Get-AzNetworkFunctionTrafficCollector.md. Please have a look. Also, we'd better providing examples for new parameter sets.

[isra-fel]

If "location" is mandatory it should be declared as required in swagger.

Make sure to autorest and ./build-module.ps1 to refresh everything after you update readme. The simplest way to not miss any file is to check in everything, because we already exclude unwanted files by .gitignore.

[kukulkarni1]

If "location" is mandatory it should be declared as required in swagger.

Make sure to autorest and ./build-module.ps1 to refresh everything after you update readme. The simplest way to not miss any file is to check in everything, because we already exclude unwanted files by .gitignore.

I have run autorest and build-modules and checked in all the files. I have made the changes in readme as well as docs and examples. I have added custom scripts for adding the location property as @BethanyZhou suggested. (in swagger we just made it a tracked resouce) It looks good now.

[BethanyZhou]

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

[kukulkarni1]

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]

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

[kukulkarni1]

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]

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

[isra-fel]

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

[kukulkarni1]

@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]

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