Skip to content

Adding new parameters for CustomIpPrefix #15333

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

Merged
merged 7 commits into from
Jul 1, 2021
Merged

Adding new parameters for CustomIpPrefix #15333

merged 7 commits into from
Jul 1, 2021

Conversation

gitlwh
Copy link
Contributor

@gitlwh gitlwh commented Jun 22, 2021

Description

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)
      • {Please put the link here}
    • the markdown help files have been regenerated using the commands listed here

@gitlwh gitlwh requested a review from MikhailTryakhov as a code owner June 22, 2021 23:31
Copy link
Contributor

@BethanyZhou BethanyZhou left a comment

Choose a reason for hiding this comment

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

Please add change log for your change.

public SwitchParameter Decommission { get; set; }

[Parameter(Mandatory = false, HelpMessage = "Run cmdlet in the background")]
public SwitchParameter Provision { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

  • I recommend using bool as the type of Provision so that we needn't two parameters for one function.
  • Please update the help message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

type would change behavior of our command because we need to use like
'''-Provision $true''' right?

Weiheng Li added 3 commits June 23, 2021 11:34
@BethanyZhou
Copy link
Contributor

Hi @gitlwh , How about Provision and Deprovision?

Copy link
Contributor Author

@gitlwh gitlwh left a comment

Choose a reason for hiding this comment

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

Hi @gitlwh , How about Provision and Deprovision?

What do you mean by that? I think they should have same behavior as commission and decommission. Only one of the four should exist.

public SwitchParameter Decommission { get; set; }

[Parameter(Mandatory = false, HelpMessage = "Run cmdlet in the background")]
public SwitchParameter Provision { get; set; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

type would change behavior of our command because we need to use like
'''-Provision $true''' right?

@BethanyZhou
Copy link
Contributor

Only one of the four should exist.

How to check this logic? It's in backend side?

Type would change behavior of our command because we need to use like
'''-Provision $true''' right?

Yes, I prefer this behavior because we can avoid customer using Provision and Deprovision at same time

@BethanyZhou BethanyZhou added needs-author-feedback More information is needed from author to address the issue. and removed ready to review labels Jun 25, 2021
@gitlwh
Copy link
Contributor Author

gitlwh commented Jun 25, 2021

Only one of the four should exist.

How to check this logic? It's in backend side?

Type would change behavior of our command because we need to use like
'''-Provision $true''' right?

Yes, I prefer this behavior because we can avoid customer using Provision and Deprovision at same time

If you read my code, my implementation is
if ((Commission ? 1 : 0) + (Decommission ? 1 : 0) + (Provision ? 1 : 0) + (Deprovision ? 1 : 0) > 1)
{
throw new ArgumentException(Microsoft.Azure.Commands.Network.Properties.Resources.CommissioningStateConflict);
}

And if we change the behavior of command it would be a break change right?

@gitlwh gitlwh added ready to review and removed needs-author-feedback More information is needed from author to address the issue. labels Jun 25, 2021
@BethanyZhou
Copy link
Contributor

BethanyZhou commented Jun 28, 2021

And if we change the behavior of command it would be a break change right?

  • Thank you for you quick response. Yes, It would be a breaking change if we change the behavior of Commission. Since Commission had been release, I want to discuss about the design of Provision with you.
  • I understand that you want to keep the behavior of Provision same with Commission. Your design looks acceptable but I'm wondering how to design is more friendly to customer. Will you feel better if you are customer and can set Provision $true, $null and $false by one parameter?

@@ -48,6 +48,9 @@
- `Update-AzVirtualHub`
* Updated cmdlets to expose two read-only properties of client certificate.
- `Get-AzApplicationGatewayTrustedClientCertificate`
* Updated cmdlets to add properties for new BYOIP features.
- `New-AzCustomIpPrefix`
Copy link
Contributor

Choose a reason for hiding this comment

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

Please sync your code with latest network-march branch so that your change log msg can include in upcoming release section.
image

@gitlwh
Copy link
Contributor Author

gitlwh commented Jun 29, 2021

And if we change the behavior of command it would be a break change right?

  • Thank you for you quick response. Yes, It would be a breaking change if we change the behavior of Commission. Since Commission had been release, I want to discuss about the design of Provision with you.
  • I understand that you want to keep the behavior of Provision same with Commission. Your design looks acceptable but I'm wondering how to design is more friendly to customer. Will you feel better if you are customer and can set Provision $true, $null and $false by one parameter?

No, The original way is more simple for cx to use and we don't have a plan to change this bahavior.

@gitlwh
Copy link
Contributor Author

gitlwh commented Jun 29, 2021

Added one more parameter for update operation CIDR

@BethanyZhou BethanyZhou added Breaking Change Release This PR contains breaking change needs-revision and removed ready to review labels Jun 30, 2021
@gitlwh gitlwh added ready to review and removed Breaking Change Release This PR contains breaking change needs-revision labels Jun 30, 2021
@BethanyZhou BethanyZhou merged commit c631d70 into Azure:network-march Jul 1, 2021
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.

2 participants