-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add spring cloud gateway cli #8037
Conversation
|
rule | cmd_name | rule_message | suggest_message |
---|---|---|---|
containerapp env java-component gateway-for-spring | sub group containerapp env java-component gateway-for-spring added |
||
containerapp env java-component nacos create | cmd containerapp env java-component nacos create added property is_preview |
||
containerapp env java-component nacos delete | cmd containerapp env java-component nacos delete added property is_preview |
||
containerapp env java-component nacos show | cmd containerapp env java-component nacos show added property is_preview |
||
containerapp env java-component nacos update | cmd containerapp env java-component nacos update added property is_preview |
Hi @Descatles, |
containerapp |
@@ -368,6 +368,7 @@ def load_arguments(self, _): | |||
c.argument('configuration', nargs="*", help="Java component configuration. Configuration must be in format \"<propertyName>=<value>\" \"<propertyName>=<value>\"...") | |||
c.argument('min_replicas', type=int, help="Minimum number of replicas to run for the Java component.") | |||
c.argument('max_replicas', type=int, help="Maximum number of replicas to run for the Java component.") | |||
c.argument('route_yaml', options_list=['--route-yaml', '--yaml'], help="Path to a .yaml file with the configuration of a Spring Cloud Gateway route. For an example, see https://aka.ms/gateway-for-spring-routes-yaml") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried https://aka.ms/gateway-for-spring-routes-yaml
But seems it doesn't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The link will point to our SCG doc, we will release the doc after the cli released, and make the link available.
@@ -213,7 +213,7 @@ def load_command_table(self, args): | |||
with self.command_group('containerapp job replica', is_preview=True) as g: | |||
g.custom_show_command('list', 'list_replica_containerappsjob') | |||
|
|||
with self.command_group('containerapp env java-component nacos') as g: | |||
with self.command_group('containerapp env java-component nacos', is_preview=True) as g: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why update az containerapp env java-component nacos
to preview?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nacos should be in preview status, and the preview is removed from java component level, we need add the preview back to nacos level. Just add in this pr for convenience
@@ -2352,12 +2352,12 @@ def update_java_component(cmd, java_component_name, environment_name, resource_g | |||
return java_component_decorator.update() | |||
|
|||
|
|||
def create_config_server_for_spring(cmd, java_component_name, environment_name, resource_group_name, configuration=None, unbind_service_bindings=None, service_bindings=None, min_replicas=1, max_replicas=1, no_wait=False): | |||
return create_java_component(cmd, java_component_name, environment_name, resource_group_name, JAVA_COMPONENT_CONFIG, configuration, service_bindings, unbind_service_bindings, min_replicas, max_replicas, no_wait) | |||
def create_config_server_for_spring(cmd, java_component_name, environment_name, resource_group_name, configuration=None, unbind_service_bindings=None, service_bindings=None, min_replicas=1, max_replicas=1, route_yaml=None, no_wait=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why add route_yaml
to not related commands?
You should not update them, right?
Same as update_config_server_for_spring, create_eureka_server_for_spring, update_eureka_server_for_spring, create_nacos, update_nacos, create_admin_for_spring, update_admin_for_spring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, they are not used in commands except gateway. Thanks for teaching me the usage of params, I have removed them with better coding
if self.get_argument_target_java_component_type() == JAVA_COMPONENT_GATEWAY: | ||
raise CLIInternalError("Service binding is not supported by Gateway for Spring.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this after refine the arguments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, removed.
if self.get_argument_unbind_service_bindings() is not None: | ||
if self.get_argument_target_java_component_type() == JAVA_COMPONENT_GATEWAY: | ||
raise CLIInternalError("Service binding is not supported by Gateway for Spring.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, removed.
if self.get_argument_target_java_component_type() != JAVA_COMPONENT_GATEWAY: | ||
raise CLIInternalError("The route could only be configured for Gateway for Spring.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, removed.
route_yaml_text_create = f""" | ||
springCloudGatewayRoutes: | ||
- id: "route1" | ||
uri: "https://otherjavacomponent.myenvironment.test.net" | ||
predicates: | ||
- "Path=/v1/path1" | ||
- "After=2024-01-01T00:00:00.000-00:00[America/Denver]" | ||
filters: | ||
- "SetPath=/filter1" | ||
- id: "route2" | ||
uri: "https://otherjavacomponent.myenvironment.test.net" | ||
predicates: | ||
- "Path=/v2/path2" | ||
- "After=2024-01-01T00:00:00.000-00:00[America/Denver]" | ||
filters: | ||
- "SetPath=/filter2" | ||
""" | ||
route_yaml_name_create = f"{self._testMethodName}_route_create.yml" | ||
|
||
write_test_file(route_yaml_name_create, route_yaml_text_create) | ||
self.cmd("containerapp env java-component gateway-for-spring create -g {} -n {} --environment {} --route-yaml {}".format(resource_group, gateway_name, env_name, route_yaml_name_create), checks=[ | ||
JMESPathCheck('name', gateway_name), | ||
JMESPathCheck('properties.componentType', "SpringCloudGateway"), | ||
JMESPathCheck('properties.provisioningState', "Succeeded"), | ||
JMESPathCheck('length(properties.springCloudGatewayRoutes)', 2), | ||
JMESPathCheck('properties.scale.minReplicas', 1), | ||
JMESPathCheck('properties.scale.maxReplicas', 1) | ||
]) | ||
|
||
# List Java Components | ||
java_component_list = self.cmd("containerapp env java-component list -g {} --environment {}".format(resource_group, env_name)).get_output_in_json() | ||
self.assertTrue(len(java_component_list) == 4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please create a new test, this test test_containerapp_java_component
already include too many commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, that make sense, I have put spring cloud gateway related test in new function
JMESPathCheck('name', gateway_name), | ||
JMESPathCheck('properties.componentType', "SpringCloudGateway"), | ||
JMESPathCheck('properties.provisioningState', "Succeeded"), | ||
JMESPathCheck('length(properties.springCloudGatewayRoutes)', 2), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check more properties that you set, for example: url
, predicates
, filters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, add check in create command
self.cmd("containerapp env java-component gateway-for-spring update -g {} -n {} --environment {}".format(resource_group, gateway_name, env_name, route_yaml_name_update), checks=[ | ||
JMESPathCheck('name', gateway_name), | ||
JMESPathCheck('properties.componentType', "SpringCloudGateway"), | ||
JMESPathCheck('properties.provisioningState', "Succeeded"), | ||
JMESPathCheck('length(properties.springCloudGatewayRoutes)', 0), | ||
JMESPathCheck('properties.scale.minReplicas', 1), | ||
JMESPathCheck('properties.scale.maxReplicas', 1) | ||
]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If execute command containerapp env java-component gateway-for-spring update
for a java-component that contains springCloudGatewayRoutes, what will happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! added that kind of check
# Check if the loaded YAML is a dictionary | ||
if not isinstance(yaml_scg_routes, dict): | ||
raise ValidationError('Invalid YAML provided. Please see https://aka.ms/gateway-for-spring-routes-yaml for a valid Gateway for Spring routes YAML spec.') | ||
|
||
# Ensure that 'springCloudGatewayRoutes' is present and is a list (can be empty) | ||
routes = yaml_scg_routes.get('springCloudGatewayRoutes') | ||
if routes is None: | ||
return [] | ||
|
||
if not isinstance(routes, list): | ||
raise ValidationError('The "springCloudGatewayRoutes" field must be a list. Please see https://aka.ms/gateway-for-spring-routes-yaml for a valid Gateway for Spring routes YAML spec.') | ||
|
||
# Loop through each route and validate the required fields | ||
for route in routes: | ||
if not isinstance(route, dict): | ||
raise ValidationError('Each route must be a dictionary. Please see https://aka.ms/gateway-for-spring-routes-yaml for a valid Gateway for Spring routes YAML spec.') | ||
|
||
# Ensure each route has 'id' and 'uri' fields | ||
if 'id' not in route or not route['id']: | ||
raise ValidationError(f'Route is missing required "id" field: {route} Please see https://aka.ms/gateway-for-spring-routes-yaml for a valid Gateway for Spring routes YAML spec.') | ||
|
||
if 'uri' not in route or not route['uri']: | ||
raise ValidationError(f'Route is missing required "uri" field: {route} Please see https://aka.ms/gateway-for-spring-routes-yaml for a valid Gateway for Spring routes YAML spec.') | ||
|
||
# Ensure predicates and filters are lists; set to empty lists if not provided | ||
if 'predicates' not in route: | ||
route['predicates'] = [] | ||
elif not isinstance(route['predicates'], list): | ||
raise ValidationError(f'The "predicates" field must be a list in route {route["id"]}. Please see https://aka.ms/gateway-for-spring-routes-yaml for a valid Gateway for Spring routes YAML spec.') | ||
|
||
if 'filters' not in route: | ||
route['filters'] = [] | ||
elif not isinstance(route['filters'], list): | ||
raise ValidationError(f'The "filters" field must be a list in route {route["id"]}. Please see https://aka.ms/gateway-for-spring-routes-yaml for a valid Gateway for Spring routes YAML spec.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we put these check in the RP? If you put them in CLI, if schema in the RP change, it will break CLI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sync with rp coder, we do not have such kind of check in RP, the check is still necessary for a short time period.
And from my point of view, if the schema in RP changes, it will result in either a new API version or a breaking change. In the first case, the old CLI package should still work with old api versions. However, if it's a breaking change, seems in any case we'll still need to update the CLI.
Hi @Juliehzl Can you help to take a look for this PR when you have chance? Thanks! |
with self.command_group('containerapp env java-component gateway-for-spring', is_preview=True) as g: | ||
g.custom_command('create', 'create_gateway_for_spring', supports_no_wait=True) | ||
g.custom_command('update', 'update_gateway_for_spring', supports_no_wait=True) | ||
g.custom_show_command('show', 'show_gateway_for_spring') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The list command is consistent with other Java components at the Java component level.
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
Hi @Descatles Release SuggestionsModule: containerapp
Notes
|
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
Hi @Descatles Please sync main branch. |
Can you help to handle this PR? Not sure why the CI stuck for a long time. Thank you! |
This checklist is used to make sure that common guidelines for a pull request are followed.
Related command
General Guidelines
azdev style <YOUR_EXT>
locally? (pip install azdev
required)python scripts/ci/test_index.py -q
locally? (pip install wheel==0.30.0
required)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.You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify
src/index.json
.