Skip to content

Update Azure CLI to support new version of frontdoor (2019-05-01)#963

Merged
mmyyrroonn merged 7 commits intoAzure:masterfrom
pichandwork:pichand/2019-05-01
Oct 8, 2019
Merged

Update Azure CLI to support new version of frontdoor (2019-05-01)#963
mmyyrroonn merged 7 commits intoAzure:masterfrom
pichandwork:pichand/2019-05-01

Conversation

@pichandwork
Copy link
Copy Markdown
Contributor


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

General Guidelines

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

For new extensions:

# g.generic_update_command('update', custom_func_name='update_fd_backend_pool')

with self.command_group('network front-door probe', frontdoor_sdk) as g:
g.custom_command('update', 'update_fd_health_probe_settings')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we use generic_update_command

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lot's of commands are commented out like network front-door probe create\delete. I'm the new maintainer for network part. Could you give me some context about these commands?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We dont support create/delete/update on probe, routingrules, frontend-endpoint. Initial swagger incorrectly stated that these are supported, which we fixed recently for all versions.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@pichandwork Hello. Could we use generic_update_command instead of custom_command?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@myronfanqiu My understanding is generic_update_command calls a PUT on the sub resource(in this case HealthProbeSettings). But PUT is not supported on HealthProbeSettings REST API. To update any settings in frontdoor, GET all settings from frontdoor, change the required values(HealthProbeSettings etc), then call PUT on frontdoor resource. So, thats why i had to use a custom command.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@pichandwork generic_update_command will call a GET method by design to get an instance. Then you can change the values. Then it will use a PUT method to return the instance to the server. The logic is almost same with what you write. So I think it's better to use generic_update_command . https://github.com/Azure/azure-cli/blob/dev/doc/authoring_command_modules/authoring_commands.md#generic-update-commands.
generic_update_command will expose some general arguments for user. User can set some fields by --set in case you don't expose them to user.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@myronfanqiu - Which instance do you refer to when you say GET method to get an instance? Frontdoor instance or HealthProbeSettings instance? if its later(HealthProbeSettings), then there is no GET and PUT supported on this.


with self.argument_context('network front-door', arg_group='BackendPools Settings') as c:
c.argument('enforce_certificate_name_check', arg_type=get_three_state_flag(positive_label='Enabled', negative_label='Disabled', return_label=True), help='Whether to disable certificate name check on HTTPS requests to all backend pools. No effect on non-HTTPS requests.')
c.argument('send_recv_timeout_seconds', type=int, help='Send and receive timeout on forwarding request to the backend. When timeout is reached, the request fails and returns.')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In general, DO NOT put units in argument names. ALWAYS put the expected units in the help text. Example: --duration-in-minutes should simply be --duration. This prevents the need to add more arguments later if more units are supported.
https://github.com/Azure/azure-cli/blob/dev/doc/command_guidelines.md#argument-naming-conventions


def create_fd_health_probe_settings(cmd, resource_group_name, front_door_name, item_name, path, interval,
protocol=None):
def create_fd_health_probe_settings(cmd, resource_group_name, front_door_name, item_name, probe_path, probe_interval,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this command usable before this PR?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. interval and path were options_list in _params.py
c.argument('probe_protocol', options_list='--protocol', arg_type=get_enum_type(FrontDoorProtocol), help='Protocol to use for sending probes.')
c.argument('probe_interval', options_list='--interval', type=int, help='Interval in seconds between probes.')

@yonzhan
Copy link
Copy Markdown
Collaborator

yonzhan commented Oct 1, 2019

@myronfanqiu will help you review.

@mmyyrroonn mmyyrroonn merged commit 2096ec9 into Azure:master Oct 8, 2019
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.

3 participants