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

[netappfiles] az volume replication: Modified volume create to allow data protection volumes and added replication operations #12173

Merged
merged 20 commits into from
Mar 29, 2020

Conversation

audunn
Copy link
Member

@audunn audunn commented Feb 13, 2020

History Notes:
(Fill in the following template if multiple notes are needed, otherwise PR title will be used for history note.)

[NetAppFiles] az volume create: Allow data protection volumes adding replication operations: approve, suspend, resume, status, remove


This checklist is used to make sure that common guidelines for a pull request are followed.

@yonzhan yonzhan requested a review from mmyyrroonn February 14, 2020 12:26
@yonzhan yonzhan added this to the S166 milestone Feb 14, 2020
@yonzhan
Copy link
Collaborator

yonzhan commented Feb 14, 2020

add to S166.

@haroldrandom
Copy link
Contributor

Hi, @audunn, I found two things may help:

  1. You have modified your test code but without updating YAML accordingly. You have to re-recording those recording YAML files as well.
  2. I found you are using Python 2.7? Recently, Python 2.7 has not been supported any more, could you use Python 3.5+ instead?

@audunn
Copy link
Member Author

audunn commented Feb 20, 2020

Thank you @haroldrandom I am updating with new test recordings. Had some issues getting JIT access etc. to run the tests those should be sorted now.
I can run them again f.ex. on Python 3.7.5 if that is required. Can you advice if that is needed?

Copy link
Contributor

@haroldrandom haroldrandom left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@haroldrandom haroldrandom left a comment

Choose a reason for hiding this comment

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

LGTM

@audunn
Copy link
Member Author

audunn commented Feb 27, 2020

Please advise if we need to do more on this.

@audunn
Copy link
Member Author

audunn commented Mar 5, 2020

Can someone look at this and advise if more work is needed on this.

@yonzhan yonzhan modified the milestones: S166, S167 Mar 7, 2020
audunn and others added 2 commits March 11, 2020 12:31
* {Misc.} Update CODEOWNERS (Azure#12149)

* {Packaging} Update version management policy (Azure#12095)

* [AppConfig]Add customer managed key when updating stores.  (Azure#12102)

* {Network} Connection Monitor V2 feature (Azure#12140)

* [Compute] BREAKING CHANGE: Fix Azure#10728: `az vm create`: create subnet automatically if vnet is specified and subnet not exists (Azure#12115)

* [Compute] Fix Azure#10728: `az vm create`: create subnet automatically if vnet is specified and subnet not exists

* {Compute} update test_vm_commands to fix merge conflict

* {Compute} fix test recording file for profile hybrid_2018_03_01 & hybrid_2019_03_01

* {Compute} update subnet help message & fail to create error message

* {Compute} update fail to create subnet error message

* {Compute} update test recording due to merge conflict

* [Aladdin] Parse generated examples into commands' _help.py (Azure#11716)

* [Compute] Increase robustness of vm image list (Azure#12134)

* {KeyVault} Modify command group name `private-endpoint` to `private-endpoint-connection` (Azure#12151)

* {Compute} Add missing parameter to attach disk example command (Azure#12045)

attaching unmanaged disk without --name will cause error
fix the example by adding `--name MyDataDisk`

* [AppService] Add support for v3 function apps and node 12. (Azure#11987)

* [functionapp] Add support for v3 function apps and node 12.

* Changed --version to --functions-version to help clarify version flags. Added functions version to invalid runtime version error.

* Fixed styling.

* [AppService] az webapp list-runtimes: Fix the bug for list-runtimes (Azure#12172)

* fix positional argument

* add test for test_webapp_runtimes

* {Packaging} Get rid of psutil dependency (Azure#11665)

* {Release} Auto generate history notes (Azure#12098)

* [AppService] az webapp|functionapp config ssl create: Add new commands to support create certificate (Azure#11955)

* Support for Managed Certificate

* Adding slot support

* Added unit test

* History change

* Remove history - part of PR description now

* Block calls for Free and Shared tier

* Update unit test

* Rename command to create

* Fix to error text

* {Packaging} Remove Python 2 in setup and doc (Azure#12155)

* {Core} use caseless matching for provisioning_state (Azure#12154)

* Fix az group deployment create  has an error when using large parameters.json file (Azure#12047)

* [ACR] `az acr login`: Throw a CLIError if there are errors returned by docker command (Azure#12156)

* [Backup] Fix for item level recovery flow in OLR (Azure#12118)

* Fix for item level recovery flow in OLR

* style fix

* [Backup] az backup recoveryconfig show: Add more parameters to support restoring as files  for SQL/SAP Hana (Azure#12116)

* initial commit

* updated tests

* updated history.rst

* cli style fix

* {Release} Upgrade to Azure CLI 2.1.0 (Azure#12195)

* update azure-cli version to 2.1.0

* Update HISTORY.rst

* {Package} remove requirements.py of python2

* Update commands.py

Co-authored-by: Xiaojian Xu <arrownj@126.com>

* [ARM] az resource: Improve the examples of the resource module (Azure#11981)

* {CI} Remove files related to Travis only (Azure#12203)

* {Monitor} az monitor autoscale create: add example for custom rule based on guest os. (Azure#12205)

* {azdev} Remove urllib3==1.24.2 from requirements.txt (Azure#12211)

* [AKS] fix the aks browse in cloud shell. (Azure#12174)

* {Storage} az storage share-rm: Add process_resource_group for resource group (Azure#12232)

* add validator=process_resource_group

* refine storage account validator

* add more validate

* [AKS] az aks: Fix monitoring addon and agentpool NoneType errors. (Azure#12250)

* {SignalR} Fix show command fails with unexpected error when the resource doesn't exist (Azure#12266)

* Do not copy tests dirs in docker image (Azure#12208)

* [Storage] az storage blob delete-batch: Misbehaving `--dryrun` flag (Azure#12162)

* [Storage] az storage blob delete-batch: Misbehaving `--dryrun` flag

* [Storage] az storage blob delete-batch: Misbehaving `--dryrun` flag

* [ACR] Fix: `az acr login` wrongly raise error (Azure#12282)

* [Network] az network application-gateway rewrite-rule create: support url configuration (Azure#12277)

* [Network] az network dns zone import: --zone-name will be case insensitive in the future. (Azure#12264)

* fix (Azure#12300)

* [AppService] Fix Azure#2258: Fixing issue where trying to create a webapp with certain runtimes was failing (Azure#12260)

* Fixing issue where trying to create a webapp with certain runtimes was failing

updating history

Upating to make sure NODE works as well when trying to set app_Settings

Pylint fixes

* Adding test & re-recording

* Removing the formatting changes to keep the changes to just the bug fix

* {Network} Supplemnt help message of --source-address-prefixes and --destination-address-prefixes for nsg rule creation (Azure#12321)

* {doc} Add Import Directive from docutils.parsers.rst for old API deprecation (Azure#12295)

sphinx.util.compat is deprecated since 1.6 and removed in 1.7.

* {Document} update install troubleshooting (Azure#12230)

* {Network} Fix wrong import ZoneType of DNS (Azure#12322)

* update codeowners (Azure#12201)

* {Storage} Change api version range for storage account kind (Azure#12265)

* change min_api

* change test

* [CosmosDB] Add Sql stored procedure, udf and trigger cmdlets (Azure#11999)

* Added cosmosDB sql stored procedure, udf and trigger cmdlets

* added help, fixed indentation, fixed wrong code

* fixed a typo

* indentation fix

* flake8 issues fixed

* PR comments

* changed help accordingly

* style fix

* change in help msg

* [ACS] (BREAKING CHANGE:) (az aks:) support msi changes for GF and BF for omsagent (Container monitoring)(#1) (Azure#12100)

* [ARM] az policy assignment list: Support listing policy assignments at Management Group scope (Azure#12086)

* Support listing policy assignments at MG scope

* Fix test recording

* Fix bug and add test case

* {Packaging/Ubuntu} remove cosmic packaging (Azure#12330)

* {Container monitoring} - Add case insensitive string compare for msi string (Azure#12341)

* [KeyVault] keyvault create: enable soft-delete by default (Azure#12204)

* make PE and PLS GA (Azure#12326)

* [Network]az network bastion: support bastion (Azure#12331)

* [AKS] az aks use-dev-spaces: Adding endpoint type option to use-dev-spaces command to customize the endpoint created on a controller (Azure#12028)

* [AKS] add tag for nodepool (Azure#12145)

* {Packaging/Homebrew} Remove patch when upgrade (Azure#12344)

* {Find} az find: Remove EUII (Azure#12349)

* delete azure-cli-extension (Azure#12362)

* [Compute] sig image-version: add --data-snapshot-luns (Azure#12303)

* [AppService] functionapp: Added error message to deployment command if resource group/function name invalid (Azure#12318)

* [AppService] fixing flag cited in warning message (Azure#12364)

* [ARM] Refactor deployment commands (Azure#10751)

* update SDK version.

* Refactor az deployment commands.

* record tests.

* test recordings.

* [ACR] Add new command `az acr helm install-cli` (Azure#12336)

* [Network] az network vnet list-available-ips: support list available ips in a vnet (Azure#12371)

* Validate ip-address parameter + tests (Azure#12312)

* [AppService] functionapp: Updated container image configuration for Linux apps (Azure#12317)

* Updated linuxFxVersion configuration for linux apps.

* Fixed HISTORY.rst, improved error message for invalid runtimes.

* Changed how linuxFxVersion is handled for dotnet. Renamed constants for increased clarity.

* [Network] Add new commands to manage flow-log and deprecate old configure command (Azure#12350)

* {Packaging/windows pip} Use local python for az.bat (Azure#12323)

* [Storage] az storage account network-rule add (bug fix): add operation should be idempotent (Azure#12327)

* [Storage] az storage account network-rule add (bug fix): add operation should be idempotent

* [Storage] az storage account network-rule add (bug fix): add operation should be idempotent

* [Storage] az storage account network-rule add (bug fix): add operation should be idempotent

* [Storage] az storage account network-rule add (bug fix): add operation should be idempotent

* [AKS] Add support of creating private cluster (Azure#12353)

* [SQL] Sql midb Add: list-deleted, show-deleted, update-retention, show-retention (Azure#12033)

* Adding support for short term retention and deleted database for MI

* Fixing help file

* Fixing help file

* Updating tests

* Adressing comments

* Fixing style errors

* Fixing help files

* Fixing help files

* Resyncing required for tests

* Chaning deletion-time to deleted-time

* More details in description on retention_days

* Adressing comments

* {KeyVault} Fix the case sensitive issue while running commands without specifying resource group name (Azure#12405)

* {Storage} az storage file copy start: add examples for snapshot Azure#12410

* {Documentation}Command guideline for private endpoint connection and private link resource (Azure#12403)

* [AKS] add support for creation time node labels (Azure#12418)

* move propagate_env_change.exe to storage account (Azure#12401)

* {Telemetry} Disable telemetry for some clouds (Azure#12400)

* [Storage] az storage account create/update: Add Routing Preference support (Azure#12309)

* add routing preferences

* add test

* fix style and rerun test

* add tests and refine code

* Add test for false

* {Monitor}show command should return 3 (Azure#12404)

* {Find} Suppress old find extension (Azure#12432)

* [Storage] Upgrade azure-mgmt-storage version to 8.0.0 (Azure#12437)

* [ACR]: private link and CMK support (Azure#12381)

* [AppService] Fix Azure#12251 app settings race condition during zip deploy (Azure#12262)

* Acr: remove private .wheel file (Azure#12450)

* [RDBMS] Support Private Endpoint Connections (Azure#12394)

* [Compute] ppg show: add --colocation-status to Enable fetching the colocation status of all the resources in the proximity placement group (Azure#12248)

* [Compute] vm/vmss/availability-set update: add --ppg to allowing updating ProximityPlacementGroup

* stage

* add test

* Add help; Support translataion from name to ID

* [Compute] ppg show: add --colocation to Enable fetching the colocation status of all the resources in the proximity placement group

* Add test

* update parameter name

* test

* revolve some comments

* Update parameter

* compute 11.0.0

* test

* fix test

* fix

* {Compute} fix none check when list vmss extension. (Azure#11914)

* [AKS] add missing / in the dashboard url. (Azure#12411)

* add allowProtectedAppendWrite (Azure#12448)

* [Monitor] az monitor metrics alert create: support `~` in `--condition`. (Azure#12439)

* update codeowners (Azure#12453)

* Improve the help of az policy assignment create (Azure#12343)

* Fix the bug that automatically generated name of policy assignment exceeds the limit (Azure#12352)

* [Cosmos DB] az cosmosdb create: add --key-uri to support adding key vault encryption information (Azure#12417)

* adding key-value-key-uri to cosmos db

* remove history note

* update package in requirements

* remove whitespace on blank line

* remove whitespace from blank line

* add preview flag and update test

* re-running tests with new API version

* rename new parameter

* [ARM] az group deployment create: Add parameter `--aux-tenants` to support cross tenants (Azure#11765) (Azure#12221)

* Add parameter --aux-tenants to support cross tenants for az group deployment create

* Add cross tenant support to the refactored deployment group create

* [ACR] add 'private-link-resource list' command (Azure#12454)

* {Documentation}fix documenttation (Azure#12435)

* [AKS] Support create aks clusters enabling managed identity (Azure#12420)

* Support create aks clusters enabling managed identity

* Remove 'preview'

* Add test case for AKS using managed identity

* [CDN] Add CDN WAF commands (Azure#12071)

* [Core] az cloud show: add insights telemetry channel endpoint for China/US cloud (Azure#12442)

* [Storage] Add support for private link resource (Azure#12383)

* initialize privete link for storage

* refine command with help

* refine help

* pass test_storage_account_private_link

* add exception handler

* add exception handler

* pass test

* fix style

* change list-private-link-resource to private-link-resource list

* fix style and linter

* apply validator and transform in core

* enabled id_part for private linke resource list

* remove previous  validator and transform in storage

* fix style

* pass test

* add ids for private link resource list

* resolve comments

* remove --ids to make linter pass

* [Compute] az vmss create/update: support automatic repairs (Azure#12374)

* [Compute] az vmss create/update: support automatic repairs

* {Compute} fix automatic repairs style error

* {Compute} update help message for --automatic-repairs-grace-period

* {Compute} update validator error message for automatic repairs

* {Compute} add automatic repairs arg group in vmss update

* {Compute} fix sytle error in _params.py

* [AKS] Validate network plugin to be either "azure" or "kubenet". (Azure#12376)

* [AppService] Fix #5720946: az webapp backup fails to set name (Azure#11929)

* Fix backup name set functionality

* Add test for backup

* changes to meet checkstyles

* add recording

* [RBAC] az ad group show: fix --group value treated as regex problem (Azure#12426)

* [RBAC] az ad group show: fix --group value treated as regex problem

* {RBAC} update syntax error of error message

* {RBAC} retrieve domain from az ad signed-in-user show instead of hard code

* {RBAC} refactor validate_group function

* {RBAC} update recording file for test_graph_group_idempotent

* {RBAC} update test_graph

* {RBAC} update test_graph to use the right user info

Co-authored-by: Xiaojian Xu <xiaojxu@microsoft.com>

* [Storage] Add PITR support (Azure#12372)

* add action for blob range

* add -t for time_to_restore

* add restore_policy properties

* rename restore_retention_days to restore_days

* try to add test

* add support_no_wait

* refine test

* fix style

* enable no wait for restore

* pass live test

* fix comments and make blob range opetional

* fix style

* {KeyVault} Modify private link commands to align with storage (Azure#12457)

* {Core} Change help example hook (Azure#12431)

* [AKS] Add aad session key support. (Azure#12290)

* {Release} use pat for github requests (Azure#12474)

* {Packaging} bump up pyyaml (Azure#12440)

* [RDBMS] Updating RDBMS Private Endpoint Tests (Azure#12475)

* [Compute] image builder create: add --image-template\n[Compute] [BREAKING CHANGE] image template: rename template to builder (Azure#11865)

* Add test for latest profile

* [Compute] image template create: add --customize and --distribute

* Add history

* Fix style

* Update test

* rename template to builder

* update help

* add image_template

* --image-template

* test

* remove --customize and --distribute

* try-catch json error

* help

* fix style

* fix a bug; update help

* test of local file

* Add example

* error handle

* help

* [SQL] az sql server create/update: Add --enable-public-network to support PublicNetworkAccess (Azure#12382)

* Add PublicNetworkAccess to Create and Update Server

* Fix some styling + rerecorded tests

* Fix more CLI style

* Cleaned up code according to comments

* Change public-network-access to enable-public-network

* Bump up azure-mgmt-sql version

* Rerecorded a couple of failing tests

* Rerecord a couple more tests

* Forgot to update test_sql_commands

* Rerecord another test

* Updating more tests

* Small changes + more rerecorded tests

* Fix style check errors

* Random small change to rerun tests

* Replace API versions

* Fix style

* [SQL DB, SQL MI] Add minimal_tls_version property for MI and SQL DB (Azure#12414)

* Managed Instance commands updated with new property

* Added enum defs for input parameter and updated Sql Server arg

* fix blank line style error

* Add minimal_tls_version for sql db server

* adding tests

* Adding recordings for tests

* Re-record mi db test

* Bump dependency version on azure-mgmt-sql, fix lint errors

* Fix code style/lint errors

* re-recorded tests

* record tets

* reset some of the test fixes and added recordings

* reset some of the test fixes and added recordings

* retry

* style

Co-authored-by: ziwa-msft <ziwa@microsoft.com>

* [AppConfig] Unblock using appconfig kv set to add keyvault reference and feature flag (Azure#12377)

* {Release} Upgrade to Azure CLI 2.2.0 (Azure#12486)

* {Document} Fix dead documentation link to Microsoft open source page (Azure#12481)

* {Compute} Delay vm image accept-terms expiration (Azure#12487)

* {Packaging} Use python3 abspath in az script. (Azure#12467)

* add test and fix 12387 (Azure#12518)

* {CDN} Delay importing ErrorResponseException (Azure#12535)

Co-authored-by: Feng Zhou <55177366+fengzhou-msft@users.noreply.github.com>
Co-authored-by: Shuai Wang <shuawan@microsoft.com>
Co-authored-by: Jianhui Harold <haroldrandom@gmail.com>
Co-authored-by: Xiaojian Xu <arrownj@126.com>
Co-authored-by: Feiyue Yu <fey@microsoft.com>
Co-authored-by: Bin Ma <bim@microsoft.com>
Co-authored-by: Gao Ruifeng <gaoruifeng@users.noreply.github.com>
Co-authored-by: Graham Zuber <grzuber@microsoft.com>
Co-authored-by: Zunli Hu <zuh@microsoft.com>
Co-authored-by: Viacheslav Vasilyev <avoidik@gmail.com>
Co-authored-by: Mads Damgård <madsd@microsoft.com>
Co-authored-by: Xing Zhou <Zhou.Xing@microsoft.com>
Co-authored-by: Lixia (Sylvia) Lei <lixia_lei@outlook.com>
Co-authored-by: Sambit Rath <es15btech11015@iith.ac.in>
Co-authored-by: Azure CLI Bot <azclibot@microsoft.com>
Co-authored-by: MyronFanQiu <49134743+MyronFanQiu@users.noreply.github.com>
Co-authored-by: Jiashuo Li <jiasli@microsoft.com>
Co-authored-by: Liming Liu <andyliuliming@outlook.com>
Co-authored-by: Jun Sun <33297523+JunSun17@users.noreply.github.com>
Co-authored-by: stan-sz <37585349+stan-sz@users.noreply.github.com>
Co-authored-by: Sylvain Rabot <sylvain@abstraction.fr>
Co-authored-by: qianwens <37290631+qianwens@users.noreply.github.com>
Co-authored-by: Sisira Panchagnula <panchagnula@users.noreply.github.com>
Co-authored-by: Luca Boccassi <luca.boccassi@gmail.com>
Co-authored-by: Yunge Zhu <37337818+yungezz@users.noreply.github.com>
Co-authored-by: Meha Kaushik <kaushik.meha@gmail.com>
Co-authored-by: rashmichandrashekar <rashmy@microsoft.com>
Co-authored-by: Chris Eggert <pilor@users.noreply.github.com>
Co-authored-by: rakeshvanga <33327237+rakeshvanga@users.noreply.github.com>
Co-authored-by: Qingqing <qizhe@microsoft.com>
Co-authored-by: Matthew Booe <mirdaki@users.noreply.github.com>
Co-authored-by: Brandon H <brandonh-msft@users.noreply.github.com>
Co-authored-by: Tiano2017 <31759375+Tiano2017@users.noreply.github.com>
Co-authored-by: Pengfei Ni <feiskyer@users.noreply.github.com>
Co-authored-by: djnisic <djnisic@microsoft.com>
Co-authored-by: Xiaofang Zhang <xizha@microsoft.com>
Co-authored-by: Yugang Wang <yugangw@microsoft.com>
Co-authored-by: yonzhan <54048360+yonzhan@users.noreply.github.com>
Co-authored-by: Xiaojian Xu <xiaojxu@microsoft.com>
Co-authored-by: Ramkumar Chandrasekaran <ramnov@users.noreply.github.com>
Co-authored-by: emgu-ms <32528205+emgu-ms@users.noreply.github.com>
Co-authored-by: Andrija Cicovic <60783126+cicovica@users.noreply.github.com>
Co-authored-by: ziwa-msft <ziwa@microsoft.com>
Co-authored-by: Jacob Bundgaard <jacob@jacobbundgaard.dk>
Co-authored-by: Matthew Ryan <mjr@centauri.org>
Co-authored-by: Audunn Baldvinsson <audunn.baldvinsson@netapp.com>
@audunn audunn requested a review from arrownj as a code owner March 11, 2020 13:30
@audunn
Copy link
Member Author

audunn commented Mar 11, 2020

Hi, This has been sitting for a while but we found out we need to update this PR changing the command pause to suspend.

Explanation is that this is also implemented in Azure Powershell and we found that that pause is not on the list for approved verbs for Cmdlets in Powershell according to cmdlet best practices document.

To avoid confusion we would like to use the same verbs so I will update this with the change shortly.

audunn and others added 2 commits March 11, 2020 14:54
@audunn audunn changed the title [netappfiles] az volume replication: Modified volume create to allow data protection volumes and added replication operations, approve, pause, resume, status and remove [netappfiles] az volume replication: Modified volume create to allow data protection volumes and added replication operations Mar 11, 2020
- name: --name --volume-name -n -v
short-summary: The name of the replication source volume
- name: --remote-volume-resource-id -d
short-summary: The resource id of the destination replication volume
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we both support destination volume name and resource id here?

Copy link
Member Author

Choose a reason for hiding this comment

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

The remote-volume-resource-id parameter expects the resource id.

- name: --volume-type
short-summary: Whether the volume should be a data protection volume ("DataProtection")
- name: --endpoint-type
short-summary: Whether the volume is source ("src") or destination ("dst")
Copy link
Contributor

@Juliehzl Juliehzl Mar 13, 2020

Choose a reason for hiding this comment

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

Can you use get_enum_type() for allowed values?

Copy link
Member Author

Choose a reason for hiding this comment

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

I suggest we do this change for next version/update that will come very soon.

az netappfiles volume replication remove -g mygroup --account-name myaccname --pool-name mypoolname --name mydestinationvolname
"""

helps['netappfiles volume replication status'] = """
Copy link
Contributor

Choose a reason for hiding this comment

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

What about changing to show command?

Copy link
Member Author

Choose a reason for hiding this comment

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

I suggest we do this change for next version/update that will come very soon.

exception_handler=netappfiles_exception_handler)
g.command('suspend', 'break_replication')
g.command('resume', 'resync_replication')
g.command('remove', 'delete_replication')
Copy link
Contributor

Choose a reason for hiding this comment

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

what about changing to show command and provide confirmation kwargs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unless I am misunderstanding I am not sure show is appropriate here this is a custom command that performs an action on approves/authorize the replication connection on the source volume.

self.cmd("az netappfiles account create -g %s -a %s -l %s" % (rg, account_name, LOCATION)).get_output_in_json()
self.cmd("az netappfiles pool create -g %s -a %s -p %s -l %s %s" % (rg, account_name, pool_name, LOCATION, POOL_DEFAULT)).get_output_in_json()
self.setup_vnet(subnet_rg, vnet_name, subnet_name, '10.0.0.0', RG_LOCATION)
self.cmd("az netappfiles account create -g %s -a %s -l %s" % (rg, account_name, ANF_LOCATION)).get_output_in_json()
Copy link
Contributor

Choose a reason for hiding this comment

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

For all your tests, can we add some checks or assert for command results?

Copy link
Member Author

Choose a reason for hiding this comment

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

I suggest we do this change for next version/update that will come very soon.


# delete
self.cmd("az netappfiles volume replication remove -g %s -a %s -p %s -v %s" % (rg_r, account_name_r, pool_name_r, volume_name_r))
time.sleep(2)
Copy link
Contributor

Choose a reason for hiding this comment

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

why sleep here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is needed to ensure this completes before any cleanup after else it mayl try and delete the resources before they are ready.

Copy link
Contributor

@Juliehzl Juliehzl left a comment

Choose a reason for hiding this comment

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

In summary, I have some concern for your PR:

  1. I will prefer you move parameter definitions to _params.py and provide available values for your some types related parameter.
    You can abstract common parameters using CLIArgumentType and reuse them in commands.
  2. For existing volume create command, it is a long running operation, right? If yes, I will recommend you support no wait for such commands and add wait command.
  3. For all your tests, it will be better to add checks.

@audunn
Copy link
Member Author

audunn commented Mar 13, 2020

In summary, I have some concern for your PR:

1. I will prefer you move parameter definitions to _params.py and provide available values for your some types related parameter.
   You can abstract common parameters using CLIArgumentType and reuse them in commands.

2. For existing volume create command, it is a long running operation, right? If yes, I will recommend you support no wait for such commands and add wait command.

3. For all your tests, it will be better to add checks.

@Juliehzl Your points are valid and appreciated. Note that [netappfiles] is in preview and marked as such.
This has been waiting for a while now, and we will need to get the next version out very soon, therefore I suggest we implement those recommendations in the next version if acceptable.

@fengzhou-msft
Copy link
Member

@audunn I flattened the history notes in PR description as the current script does not support multiple lines for one history note.

@audunn
Copy link
Member Author

audunn commented Mar 25, 2020

Hi this has been sitting for a while, what is needed to get this merged.

@yonzhan
Copy link
Collaborator

yonzhan commented Mar 28, 2020

@audunn @Juliehzl There are several conflicting files in this PR.

@audunn
Copy link
Member Author

audunn commented Mar 28, 2020

@yonzhan Apparently Network was updated in the meantime causing this conflict. Has been resolved.

Copy link
Contributor

@Juliehzl Juliehzl left a comment

Choose a reason for hiding this comment

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

Approve this PR to catch the release, but have some suggestions left which hope you can update in next version soon.

@Juliehzl Juliehzl merged commit f0ac9c5 into Azure:dev Mar 29, 2020
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.

6 participants