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

{Network} Connection monitor V2 preview #1238

Merged
merged 47 commits into from
Feb 4, 2020

Conversation

haroldrandom
Copy link
Contributor

@haroldrandom haroldrandom commented Feb 2, 2020

Temporary hotfix Azure/azure-cli#9432


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

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

Jianhui Harold added 30 commits January 16, 2020 10:31
@haroldrandom haroldrandom self-assigned this Feb 2, 2020
@azuresdkci
Copy link

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/haroldrandom/azure-cli-extensions.git@connection-monitor-v2#subdirectory=src/$EXT&egg=$EXT"

@haroldrandom haroldrandom marked this pull request as ready for review February 2, 2020 16:55
Copy link
Contributor

@mmyyrroonn mmyyrroonn left a comment

Choose a reason for hiding this comment

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

Good job! LGTM in general. Could we move one more test from CLI core in here. There is a test called test_network_watcher_connection_monitor in main CLI. I saw several tests about V2 cm which is awesome. Better to test some cross situations so that we won't bring breaking change for current users.

src/connection-monitor-preview/README.md Outdated Show resolved Hide resolved
connection_monitor_name,
watcher_rg,
watcher_name,
resource_group_name=None,
Copy link
Contributor

Choose a reason for hiding this comment

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

want to confirm. Is resource_group_name mandatory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, This is a weird parameter...Even though it was set in function signature through process_nw_cm_create_namespace(cmd, namesapce) in _validators.py, but eventually it's useless, it uses watcher_rg finally in create_nw_connection_monitor.

So I am also get confused by the old code.

c.extra('location', get_location_type(self.cli_ctx), required=True)
c.argument('resource_group_name', arg_type=ignore_type, validator=nw_validator)

with self.argument_context('network watcher connection-monitor create', arg_group='V1') as c:
Copy link
Contributor

Choose a reason for hiding this comment

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

should it be arg_group='V1 Endpoint'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I re-group them,
V1 Endpoint Arguemnt incldues: --source-xxxx, --dest-xxx, --do-not-start, --monitoring-interval
V2 Argument includes only: --notes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I re-group them:
image

help='Create the connection monitor but do not start it immediately.')
# c.ignore('location')

with self.argument_context('network watcher connection-monitor', min_api='2019-11-01') as c:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used both in V1 and V2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. It is used to create V2 only. What do you suggest?

Copy link
Contributor

Choose a reason for hiding this comment

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

arg group maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I re-group them,
V1 Endpoint Arguemnt incldues: --source-xxxx, --dest-xxx, --do-not-start, --monitoring-interval
V2 Argument includes only: --notes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I re-group them:
image

nw_connection_monitor_sdk,
client_factory=cf_nw_connection_monitor,
min_api='2018-01-01') as g:
g.custom_command('create', 'create_nw_connection_monitor', validator=process_nw_cm_create_namespace)
Copy link
Contributor

Choose a reason for hiding this comment

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

could we add more help information in _help.py for this command? like what we need to create a V2 cm. I mean in long summary part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you be more specifically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Long summary here:
image

Copy link
Contributor

@mmyyrroonn mmyyrroonn left a comment

Choose a reason for hiding this comment

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

Other part is ok. If we meet more issues, we can fix them in our main CLI

src_resource_name=source_name,
src_resource_id=source_resource_id)

# connection monitor V2 is in preview stage
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello. Is this by purpose. should it success?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As talked at Teams, believe it could be some transient issue of conflict error with ARM/LA.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change default location to eastus in test

@yonzhan yonzhan added this to the S165 milestone Feb 4, 2020
@yonzhan yonzhan requested a review from jsntcy February 4, 2020 04:16
@yonzhan
Copy link
Collaborator

yonzhan commented Feb 4, 2020

add to S165.

@haroldrandom haroldrandom merged commit 0284ef6 into Azure:master Feb 4, 2020
@haroldrandom haroldrandom deleted the connection-monitor-v2 branch February 4, 2020 07:34
ManuInNZ pushed a commit to ManuInNZ/azure-cli-extensions that referenced this pull request Apr 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants