Express Routes Cross Connection #230

tjprescott · 2018-07-13T20:44:32Z

Adds cross-connection commands under the Network model to allow service providers to managed customer ExpressRoute circuits.

Closes Azure/azure-cli#6449

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

General Guidelines

Have you run ./scripts/ci/test_static.sh locally? (pip install pylint flake8 required)
Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

My extension description/summary conforms to the Extension Summary Guidelines.

azuresdkci · 2018-07-13T20:44:54Z

If this PR is for a new extension or change to an existing extension, use the following to try out the changes in this PR:

docker run -it microsoft/azure-cli:latest
export EXT=<NAME>
pip install --upgrade --target ~/.azure/cliextensions/$EXT "git+https://github.com/Azure/azure-cli-extensions.git@ERCrossConnection#subdirectory=src/$EXT&egg=$EXT"

williexu · 2018-07-16T21:22:46Z

@tjprescott can you rebase off master? that will unblock the static checking in the CI from timing out.

williexu

Is it possible to shorten the name of this extensions? It's quite long.
Also, though not required, do consider adding tests.

williexu · 2018-07-16T21:26:16Z

src/express-route-cross-connection/azext_expressroutecrossconnection/sdk/__init__.py

+
+__all__ = ['NetworkManagementClient']
+
+__version__ = VERSION


Please put the autogenerated sdks being vendored under a folder named vendored_sdks, this will stop static checking of those files.

This did not stop static checking of the files. Static check failed because files under vendored_sdks didn't have license headers...

williexu · 2018-07-16T21:37:22Z

src/express-route-cross-connection/setup.py

+from codecs import open
+from setuptools import setup, find_packages
+
+VERSION = "0.0.7"


You probably want 0.1.0 if this will be added to the index.
If this will be a work in progress kind of thing, 0.0.1

williexu · 2018-07-16T21:41:52Z

src/express-route-cross-connection/setup.py

+    long_description='Support for ExpressRoute cross connection resources',
+    license='MIT',
+    author='Travis Prescott',
+    author_email='[email protected]',


author='Microsoft Corporation', author_email='[email protected]',

Definitely add yourself as the codeowner of this extension tho

williexu · 2018-07-16T21:44:30Z

src/express-route-cross-connection/setup.py

+    license='MIT',
+    author='Travis Prescott',
+    author_email='[email protected]',
+    url='https://github.com/Azure/azure-cli-extensions',


You may want to point to https://github.com/Azure/azure-cli-extensions/tree/master/src/express-route-cross-connection, which will allow people to directly see your extension's readme.md.

tjprescott · 2018-07-17T15:44:13Z

@williexu made the requested changes. I'm not adding tests because I could only test this manually on a special subscription that I would not have access to for re-recording tests.

I'm open to shortening the name if it doesn't sacrifice clarity. er-cross-connection or just cross-connection might be sufficient.

@karthikananth do you have a preference or suggestion?

girishmotwani

Why does this patch include changes for several other network features : express route circuit, route filter, nsg, vpn gateway etc ? Can you please clarify ? We cannot merge such a big change in one commit. Please break it into manageable commits based on the resource type

girishmotwani · 2018-07-19T21:56:24Z

src/express-route-cross-connection/azext_expressroutecrossconnection/_help.py

+helps['network cross-connection'] = """
+    type: group
+    short-summary: Manage customers' dedicated private network fiber connections to Azure.
+    long-summary: >


Would be better to use consistent terminology. Lets use Express Route Circuit.

girishmotwani · 2018-07-19T21:57:59Z

src/express-route-cross-connection/azext_expressroutecrossconnection/_help.py

+    short-summary: Show the current Address Resolution Protocol (ARP) table of an ExpressRoute circuit.
+    examples:
+        - name: Show the current Address Resolution Protocol (ARP) table of an ExpressRoute circuit.
+          text: |


Summary should be ExpressRoute circuit peering

girishmotwani · 2018-07-19T21:59:15Z

src/express-route-cross-connection/azext_expressroutecrossconnection/_help.py

+    short-summary: List available ExpressRoute service providers.
+    examples:
+        - name: List available ExpressRoute service providers.
+          text: az network cross-connection list-service-providers


This command seems incorrect. I dont think there is a rest api for this in the backend

You're right. This was copy-pasta from the normal ER commands' help.

girishmotwani · 2018-07-20T17:48:35Z

src/express-route-cross-connection/azext_expressroutecrossconnection/_params.py

+        c.argument('sku_family', sku_family_type)
+        c.argument('sku_tier', sku_tier_type)
+        c.argument('bandwidth_in_mbps', options_list=('--bandwidth',), help="Bandwidth in Mbps of the circuit.")
+        c.argument('service_provider_name', options_list=('--provider',), help="Name of the ExpressRoute Service Provider.")


service_provider_name should not be an argument for the cross connection

the list of arguments for cross connection here seem incorrect. Where did you get this from ?

They are simply copied from the relevant section of the base ExpressRoute commands to ensure consistency. If a command does not have the argument in question, the CLI ignores it.

However, I believe I have removed the irrelevant entries.

girishmotwani · 2018-07-20T18:28:41Z

src/express-route-cross-connection/azext_expressroutecrossconnection/custom.py

+
+def update_express_route_cross_connection(instance, provisioning_state=None, notes=None, bandwidth_in_mbps=None):
+
+    if bandwidth_in_mbps is not None:


The provider is not allowed to change the bandwidth for the circuit.

@karthikananth from our discussion I was left with the impression that the provider could change this parameter.

Karthik clarified and I have removed this parameter.

tjprescott · 2018-07-20T20:03:46Z

@girishmotwani the files you are referring to are the vendored SDK files that are autogenerated. You don't need to review any of that.

tjprescott · 2018-07-20T20:09:18Z

@williexu please re-review. Your comments have been addressed. I'm not inclined to shorten the extension name without the concurrence of the ExpressRoutes team.

williexu

LGTM, I'll let the network team comment about the extension contents

gimotwanMSFT

thank you for addressing review comments.

* Kubernetes Data Protection Extension CLI (#173) * First draft for Data Protection K8s backup extension (Pending internal review) * Removing tracing * Minor changes to improve azdev style * Internal PR review feedback Co-authored-by: Rishabh Raj <[email protected]> * {AKS - ARC} fix: Update DCR creation to Clusters resource group instead of workspace (#175) * fix: Update DCR creation to Clusters resource group instead of workspace * . * . * casing check * Add self-signed cert to fix PR gate for azureml extension * adding the api version to the operation definition in the client factory * bump k8s-extension version to 1.3.6 * adding tests for all 4 extension types calls * adding to test config file * updating the api version for extension types to be the correct version expected by the service * add test case for flux extension (#184) * bump k8s-extension version to 1.3.6 * bump k8s-extension version to 1.3.6 * adding upstream test for extension types * updating history.rst * [Dapr] Prompt user for existing Dapr installation during extension create (#188) * Add more validations and user prompt for existing installation scenario Signed-off-by: Shubham Sharma <[email protected]> * Add Dapr test' Signed-off-by: Shubham Sharma <[email protected]> * Handle stateful set Signed-off-by: Shubham Sharma <[email protected]> * Update default handling Signed-off-by: Shubham Sharma <[email protected]> * Fix HA handling Signed-off-by: Shubham Sharma <[email protected]> * Add placement service todo Signed-off-by: Shubham Sharma <[email protected]> * Add non-interactive mode Signed-off-by: Shubham Sharma <[email protected]> * Fix lint Signed-off-by: Shubham Sharma <[email protected]> * Update tests Signed-off-by: Shubham Sharma <[email protected]> * Reset configuration for StatefulSet during k8s upgrade Signed-off-by: Shubham Sharma <[email protected]> * Fix lint Signed-off-by: Shubham Sharma <[email protected]> * Retrigger tests Signed-off-by: Shubham Sharma <[email protected]> * Add changes to manage ha and placement params Signed-off-by: Shubham Sharma <[email protected]> * Update message Signed-off-by: Shubham Sharma <[email protected]> * nits Signed-off-by: Shubham Sharma <[email protected]> Signed-off-by: Shubham Sharma <[email protected]> * bump k8s-extension version to 1.3.7 * [Dapr] Disable applying CRDs during a downgrade (#193) * Add logging Signed-off-by: Shubham Sharma <[email protected]> * Lint Signed-off-by: Shubham Sharma <[email protected]> * Update log Signed-off-by: Shubham Sharma <[email protected]> * Revert applyCrds when not downgrading Signed-off-by: Shubham Sharma <[email protected]> * Update logic for removing hooks.applyCrds Signed-off-by: Shubham Sharma <[email protected]> * Revert logic Signed-off-by: Shubham Sharma <[email protected]> * Handle explicit hooks configuration Signed-off-by: Shubham Sharma <[email protected]> * Update comment Signed-off-by: Shubham Sharma <[email protected]> * re-trigger pipeline Signed-off-by: Shubham Sharma <[email protected]> Signed-off-by: Shubham Sharma <[email protected]> * ContainerInsights extension - Add dataCollectionSettings configuration settings (#200) * data collection settings * add support for dataCollectionSettings * fix indention * avoid duplicate use of json loads * remove whitespaces * fix pr feedback * Upgrade Python version from 3.6 to 3.7 (#203) * Upgrade Python version from 3.6 to 3.10 Upgrade to 3.10 for the job that runs Wheel, PyLint, Flake, etc., since 3.6 is not supported anymore by hosted-agent-software. * Upgrade to Python 3.10 from 3.6 Upgrade to 3.10 as 3.6 is not supported * Switch PyLink to 1.9.4 Switch PyLink to 1.9.4 from 1.9.5, as 1.9.5 is not supported with Python 3.10 * Use Python 3.7 for Static Analysis Use 3.7, as 3.10 does not support certain properties used by astpeephole.py that is used by Static Analysis tools * Try unpinned version of PyLint PyLint 1.9.5 doesn't work with Python 3.7. So, trying to see if it automatically pulls the latest compatible version. * Run pylint as a separate command * Update pylintrc (#204) * Update pylintrc * Update k8s-custom-pipelines.yml * Disable PyLint (#205) Disable PyLint for now, as the new version has breaking changes and requires lot more fixes * Disable PyLint on CI scripts * Fixes for script errors * Upgrade Static Analysis Python version Upgrade the Python version for Static Analysis to 3.10, from 3.7, now that PyLint is disabled * Try 3.9, as 3.10 has breaking changes for Flake8 * Remove version pinning for flake8 Try Python 3.10, without pinning flake8 to a version * Update k8s-custom-pipelines.yml * Use Python 3.8.1 & flake8 6.0.0 * Use Python 3.8 instead of 3.8.1 * Update k8s-custom-pipelines.yml * Update .flake8 Update to reflect breaking change in flake8 6.0 * Update source_code_static_analysis.py Scope static analysis tools to only k8s-extension module's source in our branch. * Update k8s-custom-pipelines.yml * Update k8s-custom-pipelines.yml * Update k8s-custom-pipelines.yml * Update pool name in StaticAnalysis To mirror what is in main of azure-cli-extensions * Update k8s-custom-pipelines.yml * Fix indentation * Update k8s-custom-pipelines.yml * Update k8s-custom-pipelines.yml * Revert changes * Revert changes * Revert changes to source_code_static_analysis.py * Update source_code_static_analysis.py * Revert changes * Use Ubuntu 20.4 for BuiltTestPublish stage * Switch to ubuntu-20.04 from latest Co-authored-by: Rishik Hombal <[email protected]> * [Dapr] Do not apply CRD hook when version is unchanged or auto-upgrade is being disabled (#201) * Update logic Signed-off-by: Shubham Sharma <[email protected]> * re-trigger pipeline Signed-off-by: Shubham Sharma <[email protected]> * re-trigger pipeline Signed-off-by: Shubham Sharma <[email protected]> Signed-off-by: Shubham Sharma <[email protected]> Co-authored-by: NarayanThiru <[email protected]> * add dummy key for amalogs as well * bump k8s-extension version to 1.3.8 * Adding GA api version 2022-11-01 exposing isSystemExtension and support for plan info * Seperate args for plan name, product and publisher * updating cassete file * updating HISTORY.rst * Deprecate longer parameter names when accepting config settings (#213) Co-authored-by: deeksha345 <[email protected]> * Release 1.3.9 * make containerinsights dcr name consistent (#211) Co-authored-by: Bavneet Singh <[email protected]> * [Dapr] Update version comparison logic to use semver based comparison (#219) * Update semver comparison Signed-off-by: Shubham Sharma <[email protected]> * Add log Signed-off-by: Shubham Sharma <[email protected]> --------- Signed-off-by: Shubham Sharma <[email protected]> * bump k8s-extension version to 1.4.0 (#220) * Revert "bump k8s-extension version to 1.4.0 (#220)" (#222) This reverts commit ffb8a95. * [k8s-extension] Update extension CLI to v1.4.0 * update release history * fix openservice mesh cli testcase issue * Zetia/fix ssl secret flag (#224) * fix bug: update operation doesn't respect sslSecret parameter * fix bug: update operation doesn't respect sslSecret parameter * fix typo * feat: public preview support for microsoft.azuremonitor.containers.metrics in ARC clusters (managed prometheus) (#227) * remove redundant extension test (#230) * ci MSI default for arc cluster (#231) * bump k8s-extension version to 1.4.2 * ContainerInsights extension - Extend dataCollectionSettings config settings with streams field (#232) * extend containerinsights datacollection settings with streams field * bug fix * fix lint issues * fix pr feedback * fix pr feedback * fix lint error * Generated files for 2023-05-01-preview * Support for 2023-05-01-preview * Rename get to show * Added ExtensionType api test cases * ContainerInsights extension - Extend dataCollectionSettings with containerlogv2 (#237) * Fix for Liniting issues * Fixing test cases * comment failing test cases * [k8s-extension] add kind tag in DCR creation (#240) * Use semver package (#241) Signed-off-by: Shubham Sharma <[email protected]> * Reverting commented test cases * Add support to skip provisioning of prerequisites for Azure Monitor K8s extensions (#234) * {ARC} fix: update logic to sanitize cluster name for dc* objects (#242) * Fix osm-arc version check for CI tags (#244) Signed-off-by: nshankar <[email protected]> Co-authored-by: nshankar <[email protected]> * New cassette file * Remove unused propeties from table format * bump k8s-extension version 1.4.3 * Add old commands back with deprecated status * Fix linting issues * Reverting changes for extensions type api * change the location for test runs and arc clusters * [k8s-extension] create new cli release - v1.4.3 (#250) * Revert "[k8s-extension] create new cli release - v1.4.3 (#250)" (#251) This reverts commit 584815d. * [k8s-extension] Update extension CLI to v1.4.3 * Drop relay sdk (#254) * update readme * remove useless snippets (#256) * [k8s-extension] Update extension CLI to v1.4.4 --------- Signed-off-by: Shubham Sharma <[email protected]> Signed-off-by: nshankar <[email protected]> Co-authored-by: Rishabh Raj <[email protected]> Co-authored-by: Rishabh Raj <[email protected]> Co-authored-by: bragi92 <[email protected]> Co-authored-by: Yue Yu <[email protected]> Co-authored-by: Deeksha Sharma <[email protected]> Co-authored-by: deeksha345 <[email protected]> Co-authored-by: Shubham Sharma <[email protected]> Co-authored-by: Bavneet Singh <[email protected]> Co-authored-by: Ganga Mahesh Siddem <[email protected]> Co-authored-by: NarayanThiru <[email protected]> Co-authored-by: Rishik Hombal <[email protected]> Co-authored-by: Amol Agrawal <[email protected]> Co-authored-by: Amol Agrawal <[email protected]> Co-authored-by: Arif Lakhani <[email protected]> Co-authored-by: Arif-lakhani <[email protected]> Co-authored-by: Zeliang Tian <[email protected]> Co-authored-by: Long Wan <[email protected]> Co-authored-by: ms-hujia <[email protected]> Co-authored-by: Niranjan Shankar <[email protected]> Co-authored-by: nshankar <[email protected]> Co-authored-by: necusjz <[email protected]>

tjprescott requested a review from williexu July 13, 2018 20:44

tjprescott requested a review from karthikananth July 13, 2018 20:44

tjprescott force-pushed the ERCrossConnection branch from 78b5543 to 385f466 Compare July 13, 2018 20:46

tjprescott added the extension/network label Jul 13, 2018

williexu suggested changes Jul 16, 2018

View reviewed changes

tjprescott force-pushed the ERCrossConnection branch from 6ee9aa8 to 8628c95 Compare July 17, 2018 15:39

tjprescott force-pushed the ERCrossConnection branch 3 times, most recently from a67d2e2 to 32a4913 Compare July 19, 2018 17:54

girishmotwani reviewed Jul 20, 2018

View reviewed changes

tjprescott force-pushed the ERCrossConnection branch from 32a4913 to 7bcfa90 Compare July 20, 2018 20:08

williexu reviewed Jul 20, 2018

View reviewed changes

williexu approved these changes Jul 20, 2018

View reviewed changes

tjprescott force-pushed the ERCrossConnection branch from 7bcfa90 to f5d6890 Compare July 20, 2018 21:52

tjprescott added 4 commits July 20, 2018 14:54

Initial work

49f2a31

Vendor SDK

9fcaaae

Fix lint errors.

f05325c

Code review comments.

54cd850

tjprescott force-pushed the ERCrossConnection branch from f5d6890 to adb4e70 Compare July 20, 2018 21:54

Code review fixes.

09e1271

tjprescott force-pushed the ERCrossConnection branch from adb4e70 to 09e1271 Compare July 20, 2018 22:20

tjprescott and others added 2 commits July 23, 2018 08:50

Merge branch 'master' into ERCrossConnection

508c035

Code review feedback.

808c72f

gimotwanMSFT approved these changes Jul 23, 2018

View reviewed changes

tjprescott merged commit bbefbe7 into master Jul 24, 2018

tjprescott deleted the ERCrossConnection branch July 24, 2018 02:40

tjprescott mentioned this pull request Aug 6, 2018

Publish ExpressRoute Cross Connection extension to Index #255

Merged

ms-hujia pushed a commit to ms-hujia/azure-cli-extensions that referenced this pull request Jun 21, 2023

remove redundant extension test (Azure#230)

4f2b74c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Express Routes Cross Connection #230

Express Routes Cross Connection #230

tjprescott commented Jul 13, 2018

azuresdkci commented Jul 13, 2018

williexu commented Jul 16, 2018 •

edited

Loading

williexu left a comment •

edited

Loading

williexu Jul 16, 2018

tjprescott Jul 17, 2018

williexu Jul 16, 2018

williexu Jul 16, 2018

williexu Jul 16, 2018

tjprescott commented Jul 17, 2018 •

edited

Loading

girishmotwani left a comment

girishmotwani Jul 19, 2018

girishmotwani Jul 19, 2018

girishmotwani Jul 19, 2018

tjprescott Jul 20, 2018

girishmotwani Jul 20, 2018

girishmotwani Jul 20, 2018

tjprescott Jul 20, 2018 •

edited

Loading

girishmotwani Jul 20, 2018

tjprescott Jul 20, 2018

tjprescott Jul 23, 2018

tjprescott commented Jul 20, 2018

tjprescott commented Jul 20, 2018

williexu left a comment •

edited

Loading

gimotwanMSFT left a comment


		def update_express_route_cross_connection(instance, provisioning_state=None, notes=None, bandwidth_in_mbps=None):

		if bandwidth_in_mbps is not None:


		__all__ = ['NetworkManagementClient']

		__version__ = VERSION

Express Routes Cross Connection #230

Express Routes Cross Connection #230

Conversation

tjprescott commented Jul 13, 2018

General Guidelines

azuresdkci commented Jul 13, 2018

williexu commented Jul 16, 2018 • edited Loading

williexu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjprescott commented Jul 17, 2018 • edited Loading

girishmotwani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjprescott Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjprescott commented Jul 20, 2018

tjprescott commented Jul 20, 2018

williexu left a comment • edited Loading

Choose a reason for hiding this comment

gimotwanMSFT left a comment

Choose a reason for hiding this comment

williexu commented Jul 16, 2018 •

edited

Loading

williexu left a comment •

edited

Loading

tjprescott commented Jul 17, 2018 •

edited

Loading

tjprescott Jul 20, 2018 •

edited

Loading

williexu left a comment •

edited

Loading