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.

Sign up for GitHub

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

Jump to bottom

[Network] Added Cmdlets for Network Virtual Appliance (NVA) and NVA site resources. #12213

Merged
merged 13 commits into from
Jun 24, 2020
Merged

[Network] Added Cmdlets for Network Virtual Appliance (NVA) and NVA site resources. #12213

merged 13 commits into from
Jun 24, 2020

Conversation

3vilbuff3r
Copy link
Contributor

Description

This pull request adds cmdlets for a new resource type "Network Virtual Appliance" in Azure.

The link to powershell design review

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

Neel Bhavsar added 7 commits June 18, 2020 23:02
Created Model Classes for NVA powershell object.
a866265
Added *-NetworkVirtualAppliance commands.
5b8d214
Added *-NVASite commands and export commands.
1431345
Added tests for NVA CRUD operations.
d86e184 
The tests for NVA Site CRUD operations is in skipped state.
Reason for this is a bug in the Nfv-RP which fails the
New-AzVirtualApplianceSite command. The fix is underway and
Nfv-RP team will record the tests and push it in a separate change.
Added help files for the new 11 Cmdlets.
01dfc0c
Added entry in the Changelog
ec77b55
Bug fix for null NVA Id.
6e4bda2
@3vilbuff3r 3vilbuff3r requested a review from anton-evseev as a code owner June 18, 2020 18:50
@adxsdkps
Copy link
Collaborator

Can one of the admins verify this patch?

@3vilbuff3r
Copy link
Contributor Author

3vilbuff3r commented Jun 19, 2020 •
edited
Loading

@number213 , I see that some validations are failing due to "online version" field being empty in the help md files. We do not have any online version of the help yet on these commands. Should I just add the expected online help page link in those fields?

Edit: I see that there is an open issue on this behavior of the platyPS and checks in the CI. Let me know the steps I need to follow to resolve this. :)

@VeryEarly
Copy link
Contributor

@number213 , I see that some validations are failing due to "online version" field being empty in the help md files. We do not have any online version of the help yet on these commands. Should I just add the expected online help page link in those fields?

Edit: I see that there is an open issue on this behavior of the platyPS and checks in the CI. Let me know the steps I need to follow to resolve this. :)

The online version should be in certain format, just paste it to help markdown.

@3vilbuff3r
Copy link
Contributor Author

Hi @VeryEarly , can you point me to the required format?. The generated online version is empty.

@VeryEarly
Copy link
Contributor

Hi @VeryEarly , can you point me to the required format?. The generated online version is empty.

should be like this:
https://docs.microsoft.com/en-us/powershell/module/az.network/{cmdlet}

VeryEarly
VeryEarly reviewed Jun 19, 2020
View reviewed changes
src/Network/Network/NetworkVirtualAppliance/NewNetworkVirtualApplianceCommand.cs Outdated

namespace Microsoft.Azure.Commands.Network
{
[Cmdlet("New", ResourceManager.Common.AzureRMConstants.AzureRMPrefix + "NetworkVirtualAppliance", SupportsShouldProcess = true), OutputType(typeof(PSNetworkVirtualAppliance))]
Copy link
Contributor

Choose a reason for hiding this comment

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

please add ResourceNameParameterSet as default parameter

VeryEarly
VeryEarly reviewed Jun 19, 2020
View reviewed changes
src/Network/Network/NetworkVirtualAppliance/Sites/GetVirtualApplianceSiteCommand.cs Outdated
public virtual string ResourceGroupName { get; set; }

[Parameter(
Mandatory = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be true

VeryEarly
VeryEarly requested changes Jun 19, 2020
View reviewed changes
Copy link
Contributor

@VeryEarly VeryEarly left a comment

Choose a reason for hiding this comment

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

please resolve comments, and re-generate help accordingly

@VeryEarly VeryEarly self-assigned this Jun 19, 2020
@3vilbuff3r
Copy link
Contributor Author

3vilbuff3r commented Jun 21, 2020 •
edited
Loading

@VeryEarly , some of the checks are failing due to the commands "New-AzOffice365PolicyProperty" and "New-AzVirtualApplianceSkuProperty". Both commands do NOT create any resources but rather create a local powershell objects to be inserted in a subsequent command. In these cases I believe the options like ShouldProcess, Force, Confirm do not apply. Let me know if my understanding is incorrect and something needs to be changed. Also, shall we change the design review issue with the new command names as well?

@VeryEarly
Copy link
Contributor

@VeryEarly , some of the checks are failing due to the commands "New-AzOffice365PolicyProperty" and "New-AzVirtualApplianceSkuProperty". Both commands do NOT create any resources but rather create a local powershell objects to be inserted in a subsequent command. In these cases I believe the options like ShouldProcess, Force, Confirm do not apply. Let me know if my understanding is incorrect and something needs to be changed. Also, shall we change the design review issue with the new command names as well?

Hi @neelbhavsar97 ,

Please suppress these two messages into https://github.com/Azure/azure-powershell/blob/master/tools/StaticAnalysis/Exceptions/Az.Network/BreakingChangeIssues.csv

Please re-target your PR to network-june branch, one simple way is to create a new branch out of network-june and cherry-pick commits into it.

@3vilbuff3r 3vilbuff3r changed the base branch from master to network-june June 22, 2020 05:43
@3vilbuff3r 3vilbuff3r changed the base branch from network-june to master June 22, 2020 05:44
@3vilbuff3r
Copy link
Contributor Author

@VeryEarly , created a new branch and a new PR. Closing this PR.

@3vilbuff3r 3vilbuff3r closed this Jun 22, 2020
@3vilbuff3r 3vilbuff3r reopened this Jun 22, 2020
@VeryEarly
Copy link
Contributor

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

@3vilbuff3r
Copy link
Contributor Author

@VeryEarly , Some checks are failing with no obvious reason related to the changes I've pushed. Is this expected or I'm missing something here?

@VeryEarly
Copy link
Contributor

@VeryEarly , Some checks are failing with no obvious reason related to the changes I've pushed. Is this expected or I'm missing something here?

Hi @neelbhavsar97 ,

These failures were caused by some recent changes in master branch, I'll take care of it.

@3vilbuff3r
Copy link
Contributor Author

Sure, let me know if you need something. You can go ahead and merge this when convenient. Thanks you.

@VeryEarly
Copy link
Contributor

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

@3vilbuff3r
Copy link
Contributor Author

Hi @VeryEarly , there is a conflict for this PR. Can you resolve this conflict as it is related to introducing a new version to the Az.Network module. Let me know if you need anything from my side.

@VeryEarly
Copy link
Contributor

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

@3vilbuff3r
Copy link
Contributor Author

Hi @number213 , please review and merge this pull request as the release date is approaching. Let me know if you need any changes.

@VeryEarly VeryEarly merged commit 10b4d7b into Azure:master Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VeryEarly VeryEarly VeryEarly approved these changes

@anton-evseev anton-evseev anton-evseev approved these changes

Assignees

@VeryEarly VeryEarly

@3vilbuff3r 3vilbuff3r

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@3vilbuff3r @adxsdkps @VeryEarly @anton-evseev @azuresdkci