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

[feat] Add support for --assign-public-endpoint and --enable-log-stream-public-endpoint #4866

Merged
merged 18 commits into from
May 26, 2022

Conversation

Descatles
Copy link
Contributor


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

Related command

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:

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update src/index.json automatically.
The precondition is to put your code inside this repository and upgrade the version in the pull request but do not modify src/index.json.

@yonzhan
Copy link
Collaborator

yonzhan commented May 21, 2022

spring

Comment on lines 5 to 10
* Command `az spring create` has new argument "--ingress-read-timeout" to set ingress read timeout when create Azure Spring App.
* Command `az spring update` has new argument "--ingress-read-timeout" to update ingress read timeout for Azure Spring App.
* Command `az spring create` has new argument "--enable-log-stream-public-endpoint" to set whether assign public endpoint for log streaming in vnet injection instance when create Azure Spring App.
* Command `az spring update` has new argument "--enable-log-stream-public-endpoint" to update whether assign public endpoint for log streaming in vnet injection instance for Azure Spring App.
* Command `az spring app create` has new argument "--assign_public_endpoint" to set whether assign endpoint URL which could be accessed out of virtual network for vnet injection instance app.
* Command `az spring app update` has new argument "--assign_public_endpoint" to update whether assign endpoint URL which could be accessed out of virtual network for vnet injection instance app.
Copy link
Contributor

Choose a reason for hiding this comment

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

It is suggested to merge some similar history notes, so that there will not be too much content for users to read

Suggested change
* Command `az spring create` has new argument "--ingress-read-timeout" to set ingress read timeout when create Azure Spring App.
* Command `az spring update` has new argument "--ingress-read-timeout" to update ingress read timeout for Azure Spring App.
* Command `az spring create` has new argument "--enable-log-stream-public-endpoint" to set whether assign public endpoint for log streaming in vnet injection instance when create Azure Spring App.
* Command `az spring update` has new argument "--enable-log-stream-public-endpoint" to update whether assign public endpoint for log streaming in vnet injection instance for Azure Spring App.
* Command `az spring app create` has new argument "--assign_public_endpoint" to set whether assign endpoint URL which could be accessed out of virtual network for vnet injection instance app.
* Command `az spring app update` has new argument "--assign_public_endpoint" to update whether assign endpoint URL which could be accessed out of virtual network for vnet injection instance app.
* Command `az spring create/update` has new argument "--ingress-read-timeout" to set ingress read timeout when create Azure Spring App.
* Command `az spring create/update` has new argument "--enable-log-stream-public-endpoint" to set whether assign public endpoint for log streaming in vnet injection instance when create Azure Spring App.
* Command `az spring app create/update` has new argument "--assign_public_endpoint" to set whether assign endpoint URL which could be accessed out of virtual network for vnet injection instance app.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@@ -76,6 +76,10 @@ def load_arguments(self, _):
c.argument('service_runtime_subnet', arg_group='VNet Injection', options_list=['--service-runtime-subnet', '--svc-subnet'], help='The name or ID of an existing subnet in "vnet" into which to deploy the Spring Apps service runtime. Required when deploying into a Virtual Network.', validator=validate_vnet)
c.argument('service_runtime_network_resource_group', arg_group='VNet Injection', options_list=['--service-runtime-network-resource-group', '--svc-nrg'], help='The resource group where all network resources for Azure Spring Apps service runtime will be created in.', validator=validate_node_resource_group)
c.argument('app_network_resource_group', arg_group='VNet Injection', options_list=['--app-network-resource-group', '--app-nrg'], help='The resource group where all network resources for apps will be created in.', validator=validate_node_resource_group)
c.argument('enable_log_stream_public_endpoint',
arg_type=get_three_state_flag(),
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 use action='store_true' instead of get_three_state_flag()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems could not, as this parameter would be persisted as properties of a resource

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks~

@zhoxing-ms
Copy link
Contributor

Could we add some scenario tests for those new parameters?

@zhoxing-ms
Copy link
Contributor

Please address those conflicts

@Descatles
Copy link
Contributor Author

Please address those conflicts

Resolved

@Descatles
Copy link
Contributor Author

Could we add some scenario tests for those new parameters?

I have added new scenario tests

@zhoxing-ms zhoxing-ms merged commit ac28513 into Azure:main May 26, 2022
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