Sobaner/wildcardlistener

[sobaner-zz]

Description

Adding Support for HostNames in Application Gateway Http Listener Class

Checklist

• I have read the Submitting Changes section of CONTRIBUTING.md
• The title of the PR is clear and informative
• The appropriate ChangeLog.md file(s) has been updated:
  • 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 -- no new version header should be added
• The PR does not introduce breaking changes
• If applicable, the changes made in the PR have proper test coverage
• For public API changes to cmdlets:
  • a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
  • the markdown help files have been regenerated using the commands listed here

[sobaner-zz]

Swagger: Azure/azure-rest-api-specs#7453
PowerShell Design PR: https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/463

[isra-fel]

"Parameter HostNames of cmdlet Add-AzApplicationGatewayHttpListener does not follow the enforced naming convention of using a singular noun for a parameter name."

Please consider renaming to "HostName" or "HostNameList"

[sobaner-zz]

"Parameter HostNames of cmdlet Add-AzApplicationGatewayHttpListener does not follow the enforced naming convention of using a singular noun for a parameter name."

Please consider renaming to "HostName" or "HostNameList"

The NRP and swagger for this name has already been in field and there are private preview customers onboarded on the property called HostNames. If I call it HostNames in Swagger and call it HostNameList in PS, doesn't that conflict? I would request you to let me go ahead with this name. Discussed it with program management team as well on the naming convention.

[isra-fel]

The NRP and swagger for this name has already been in field and there are private preview customers onboarded on the property called HostNames. If I call it HostNames in Swagger and call it HostNameList in PS, doesn't that conflict? I would request you to let me go ahead with this name. Discussed it with program management team as well on the naming convention.

It's not just our team's recommendation, but also PowerShell (the language) team's strongly encouraged development guideline to use singular parameter names.
I understand your need to make it consistent everywhere, but we need to follow the conventions in each environments as well.

I understood your concern, but per the documentation, it does not tell me about recommendations about property field naming conventions, where I could also provide a single entry, as well as a list of entries. ApplicationGatewayHttpListener always had the HostName attribute, which is allowed to take only a single dns hostname. With customer ask, we are now supporting wildcard characters, as well as multiple hostnames per listener, which we did not want to add in the HostName field (as it has singular name), which is the reason we introduced another field named HostNames
(for which we even had the swagger approved. )

Even from PS perspective, I see that some other cmdlets, like Set-AzApplicationGatewayBackendAddressPool, which had "BackendIPAddresses" and "BackendFqdns", which are basically list of strings, which can allow one or more of those categories.

Now, considering swagger getting approved, I expected this to be a valid naming convention for my use case, which is the reason for raising this PS review. Now, simply changing a name, as I have mentioned earlier means to change it in NRP, wait for release, change it in swagger and then finally change PS cmdlets, which would increase the go to market for this feature.

Currently the PR is ready from my side, apart from few merge validations failing (not related I guess), apart from review.

[isra-fel]

Hi @sobaner , thanks for the detailed explanation of the scenario, that is helpful. (BTW please don't edit-reply, it's hard to notice)
Now let's just think about powershell cmdlet interface, not the swagger. Here's my proposal:

• Do not use HostNames
• Change the type of string HostName to string[] HostName

Here's why:

1. The API surface would be simple. User doesn't need to think about using which of HostName or HostNames
2. The implementation is simple too
3. Parameter naming conforms with powershell naming convention

And you needn't worry -- this is not a breaking change in powershell, as array parameters can take a single object.
How do you like the idea?

[isra-fel]

More details on parameter naming convention: we only accept 2 types of plural names:

• Units, e.g. -TimeInMinutes
• The argument can never be single, in which way the cmdlet should check and warn the user if a single object is supplied

As of the counter examples you gave, they were probably checked in due to some historic information.

[sobaner-zz]

@number213 - I have corrected the staticAnalyzer errors for my PR, but seems like something else from vpn is failing the PR - https://dev.azure.com/azure-sdk/public/_build/results?buildId=238898&view=logs&j=e1b7b984-9f58-529f-7c5a-f15f8e35cfa6&t=e8689ae8-b216-55f7-b207-4831f84ff98a&l=98

Is there a successful build in network-december branch ?

[anton-evseev]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

The base branch was changed.