From a5bd1c53b0587e8e71cc80d907f4d06b094fcbe2 Mon Sep 17 00:00:00 2001 From: Yuwei Zhou Date: Fri, 3 Dec 2021 16:24:46 +0800 Subject: [PATCH] =?UTF-8?q?refactor:=20=F0=9F=92=A1=20Use=20validator=20to?= =?UTF-8?q?=20validate=20or=20default=20deployment=20(#4188)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In most secenarios under `az spring-cloud app`, it can accept a parameter deployment. If the deployment is set, the command will update apps/deployments resource. Otherwise, it will look for one deployment under the app meets deployment.properties.active = true to be operated. In today's code, each command handle such logic to fetch or use default value. This PR abstract this logic to validator, and make custom command can be focus on the operation logic --- .../azext_spring_cloud/_app_validator.py | 43 ++++++ .../azext_spring_cloud/_params.py | 9 +- .../azext_spring_cloud/commands.py | 1 - src/spring-cloud/azext_spring_cloud/custom.py | 127 +++--------------- .../tests/latest/test_asc_app_validator.py | 105 +++++++++++++++ 5 files changed, 173 insertions(+), 112 deletions(-) create mode 100644 src/spring-cloud/azext_spring_cloud/_app_validator.py create mode 100644 src/spring-cloud/azext_spring_cloud/tests/latest/test_asc_app_validator.py diff --git a/src/spring-cloud/azext_spring_cloud/_app_validator.py b/src/spring-cloud/azext_spring_cloud/_app_validator.py new file mode 100644 index 00000000000..c53429b908d --- /dev/null +++ b/src/spring-cloud/azext_spring_cloud/_app_validator.py @@ -0,0 +1,43 @@ +# -------------------------------------------------------------------------------------------- +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. See License.txt in the project root for license information. +# -------------------------------------------------------------------------------------------- + +# pylint: disable=too-few-public-methods, unused-argument, redefined-builtin + +from azure.cli.core.azclierror import InvalidArgumentValueError +from msrestazure.azure_exceptions import CloudError +from ._client_factory import cf_spring_cloud + + +# pylint: disable=line-too-long,raise-missing-from +NO_PRODUCTION_DEPLOYMENT_ERROR = "No production deployment found, use --deployment to specify deployment or create deployment with: az spring-cloud app deployment create" + + +def fulfill_deployment_param(cmd, namespace): + client = cf_spring_cloud(cmd.cli_ctx) + if not namespace.name or not namespace.service or not namespace.resource_group: + return + if namespace.deployment: + namespace.deployment = _ensure_deployment_exist(client, namespace.resource_group, namespace.service, namespace.name, namespace.deployment) + else: + namespace.deployment = _ensure_active_deployment_exist_and_get(client, namespace.resource_group, namespace.service, namespace.name) + + +def _ensure_deployment_exist(client, resource_group, service, app, deployment): + try: + return client.deployments.get(resource_group, service, app, deployment) + except CloudError: + raise InvalidArgumentValueError('Deployment {} not found under app {}'.format(deployment, app)) + + +def _ensure_active_deployment_exist_and_get(client, resource_group, service, name): + deployment_resource = _get_active_deployment(client, resource_group, service, name) + if not deployment_resource: + raise InvalidArgumentValueError(NO_PRODUCTION_DEPLOYMENT_ERROR) + return deployment_resource + + +def _get_active_deployment(client, resource_group, service, name): + deployments = client.deployments.list(resource_group, service, name) + return next(iter(x for x in deployments if x.properties.active), None) diff --git a/src/spring-cloud/azext_spring_cloud/_params.py b/src/spring-cloud/azext_spring_cloud/_params.py index 7cd46758d36..5c1d79f7578 100644 --- a/src/spring-cloud/azext_spring_cloud/_params.py +++ b/src/spring-cloud/azext_spring_cloud/_params.py @@ -14,6 +14,7 @@ validate_tracing_parameters_asc_create, validate_tracing_parameters_asc_update, validate_app_insights_parameters, validate_instance_count, validate_java_agent_parameters, validate_jar) +from ._app_validator import (fulfill_deployment_param) from ._utils import ApiType from .vendored_sdks.appplatform.v2020_07_01.models import RuntimeVersion, TestKeyType @@ -149,7 +150,7 @@ def load_arguments(self, _): for scope in ['spring-cloud app update', 'spring-cloud app start', 'spring-cloud app stop', 'spring-cloud app restart', 'spring-cloud app deploy', 'spring-cloud app scale', 'spring-cloud app set-deployment', 'spring-cloud app show-deploy-log']: with self.argument_context(scope) as c: c.argument('deployment', options_list=[ - '--deployment', '-d'], help='Name of an existing deployment of the app. Default to the production deployment if not specified.', validator=validate_deployment_name) + '--deployment', '-d'], help='Name of an existing deployment of the app. Default to the production deployment if not specified.', validator=fulfill_deployment_param) c.argument('main_entry', options_list=[ '--main-entry', '-m'], help="The path to the .NET executable relative to zip root.") @@ -165,7 +166,7 @@ def prepare_logs_argument(c): c.argument('since', help='Only return logs newer than a relative duration like 5s, 2m, or 1h. Maximum is 1h', validator=validate_log_since) c.argument('limit', type=int, help='Maximum kilobytes of logs to return. Ceiling number is 2048.', validator=validate_log_limit) c.argument('deployment', options_list=[ - '--deployment', '-d'], help='Name of an existing deployment of the app. Default to the production deployment if not specified.', validator=validate_deployment_name) + '--deployment', '-d'], help='Name of an existing deployment of the app. Default to the production deployment if not specified.', validator=fulfill_deployment_param) c.argument('format_json', nargs='?', const='{timestamp} {level:>5} [{thread:>15.15}] {logger{39}:<40.40}: {message}\n{stackTrace}', help='Format JSON logs if structured log is enabled') @@ -234,13 +235,13 @@ def prepare_logs_argument(c): for scope in ['spring-cloud app deployment generate-heap-dump', 'spring-cloud app deployment generate-thread-dump']: with self.argument_context(scope) as c: c.argument('deployment', options_list=[ - '--deployment', '-d'], help='Name of an existing deployment of the app. Default to the production deployment if not specified.', validator=validate_deployment_name) + '--deployment', '-d'], help='Name of an existing deployment of the app. Default to the production deployment if not specified.', validator=fulfill_deployment_param) c.argument('app_instance', help='Target app instance you want to dump.') c.argument('file_path', help='The mount file path for your dump file.') with self.argument_context('spring-cloud app deployment start-jfr') as c: c.argument('deployment', options_list=[ - '--deployment', '-d'], help='Name of an existing deployment of the app. Default to the production deployment if not specified.', validator=validate_deployment_name) + '--deployment', '-d'], help='Name of an existing deployment of the app. Default to the production deployment if not specified.', validator=fulfill_deployment_param) c.argument('app_instance', help='Target app instance you want to dump.') c.argument('file_path', help='The mount file path for your dump file.') c.argument('duration', type=str, default="60s", help='Duration of JFR.') diff --git a/src/spring-cloud/azext_spring_cloud/commands.py b/src/spring-cloud/azext_spring_cloud/commands.py index 4220afbcc6a..fa751b33a17 100644 --- a/src/spring-cloud/azext_spring_cloud/commands.py +++ b/src/spring-cloud/azext_spring_cloud/commands.py @@ -9,7 +9,6 @@ from ._client_factory import (cf_app_services, cf_spring_cloud, cf_spring_cloud_20201101preview, - cf_spring_cloud_20210601preview, cf_spring_cloud_20210901preview, cf_config_servers) from ._transformers import (transform_spring_cloud_table_output, diff --git a/src/spring-cloud/azext_spring_cloud/custom.py b/src/spring-cloud/azext_spring_cloud/custom.py index 50ca52caaba..4f49e71be35 100644 --- a/src/spring-cloud/azext_spring_cloud/custom.py +++ b/src/spring-cloud/azext_spring_cloud/custom.py @@ -481,7 +481,6 @@ def app_update(cmd, client, resource_group, service, name, enable_end_to_end_tls=None, persistent_storage=None, loaded_public_certificate_file=None): - _check_active_deployment_exist(client, resource_group, service, name) resource = client.services.get(resource_group, service) location = resource.location @@ -549,16 +548,7 @@ def app_update(cmd, client, resource_group, service, name, app_updated = client.apps.get(resource_group, service, name) - if deployment is None: - logger.warning( - "No '--deployment' given, will update app's production deployment") - deployment = client.apps.get( - resource_group, service, name).properties.active_deployment_name - if deployment is None: - logger.warning("No production deployment found for update") - return app_updated - - logger.warning("[2/2] Updating deployment '{}'".format(deployment)) + logger.warning("[2/2] Updating deployment '{}'".format(deployment.name)) container_probe_settings = None if disable_probe is not None: container_probe_settings = models_20210901preview.DeploymentSettingsContainerProbeSettings(disable_probe=disable_probe) @@ -575,12 +565,12 @@ def app_update(cmd, client, resource_group, service, name, deployment_settings=deployment_settings) deployment_resource = models.DeploymentResource(properties=properties) poller = client.deployments.begin_update( - resource_group, service, name, deployment, deployment_resource) + resource_group, service, name, deployment.name, deployment_resource) while poller.done() is False: sleep(DEPLOYMENT_CREATE_OR_UPDATE_SLEEP_INTERVAL) deployment = client.deployments.get( - resource_group, service, name, deployment) + resource_group, service, name, deployment.name) app_updated.properties.active_deployment = deployment return app_updated @@ -599,16 +589,9 @@ def app_start(cmd, client, name, deployment=None, no_wait=False): - if deployment is None: - deployment = client.apps.get( - resource_group, service, name).properties.active_deployment_name - if deployment is None: - logger.warning(NO_PRODUCTION_DEPLOYMENT_SET_ERROR) - raise CLIError(NO_PRODUCTION_DEPLOYMENT_ERROR) - logger.warning("Successfully triggered the action 'start' for the app '{}'".format(name)) return sdk_no_wait(no_wait, client.deployments.begin_start, - resource_group, service, name, deployment) + resource_group, service, name, deployment.name) def app_stop(cmd, client, @@ -617,16 +600,9 @@ def app_stop(cmd, client, name, deployment=None, no_wait=False): - if deployment is None: - deployment = client.apps.get( - resource_group, service, name).properties.active_deployment_name - if deployment is None: - logger.warning(NO_PRODUCTION_DEPLOYMENT_SET_ERROR) - raise CLIError(NO_PRODUCTION_DEPLOYMENT_ERROR) - logger.warning("Successfully triggered the action 'stop' for the app '{}'".format(name)) return sdk_no_wait(no_wait, client.deployments.begin_stop, - resource_group, service, name, deployment) + resource_group, service, name, deployment.name) def app_restart(cmd, client, @@ -635,16 +611,9 @@ def app_restart(cmd, client, name, deployment=None, no_wait=False): - if deployment is None: - deployment = client.apps.get( - resource_group, service, name).properties.active_deployment_name - if deployment is None: - logger.warning(NO_PRODUCTION_DEPLOYMENT_SET_ERROR) - raise CLIError(NO_PRODUCTION_DEPLOYMENT_ERROR) - logger.warning("Successfully triggered the action 'restart' for the app '{}'".format(name)) return sdk_no_wait(no_wait, client.deployments.begin_restart, - resource_group, service, name, deployment) + resource_group, service, name, deployment.name) def app_list(cmd, client, @@ -654,11 +623,8 @@ def app_list(cmd, client, deployments = list( client.deployments.list_for_cluster(resource_group, service)) for app in apps: - if app.properties.active_deployment_name: - deployment = next( - (x for x in deployments if x.properties.app_name == app.name)) - app.properties.active_deployment = deployment - + app.properties.active_deployment = next(iter(x for x in deployments + if x.properties.active and x.id.startswith(app.id + '/deployments/')), None) return apps @@ -692,22 +658,13 @@ def app_deploy(cmd, client, resource_group, service, name, disable_probe=None, no_wait=False): logger.warning(LOG_RUNNING_PROMPT) - if not deployment: - deployment = client.apps.get( - resource_group, service, name).properties.active_deployment_name - if not deployment: - logger.warning(NO_PRODUCTION_DEPLOYMENT_SET_ERROR) - raise CLIError(NO_PRODUCTION_DEPLOYMENT_ERROR) - - client.deployments.get(resource_group, service, name, deployment) - file_type, file_path = _get_upload_local_file(runtime_version, artifact_path, source_path) return _app_deploy(client, resource_group, service, name, - deployment, + deployment.name, version, file_path, runtime_version, @@ -732,12 +689,6 @@ def app_scale(cmd, client, resource_group, service, name, no_wait=False): cpu = validate_cpu(cpu) memory = validate_memory(memory) - if deployment is None: - deployment = client.apps.get( - resource_group, service, name).properties.active_deployment_name - if deployment is None: - logger.warning(NO_PRODUCTION_DEPLOYMENT_SET_ERROR) - raise CLIError(NO_PRODUCTION_DEPLOYMENT_ERROR) resource = client.services.get(resource_group, service) _validate_instance_count(resource.sku.tier, instance_count) @@ -752,36 +703,22 @@ def app_scale(cmd, client, resource_group, service, name, sku = models_20210601preview.Sku(name="S0", tier="STANDARD", capacity=instance_count) deployment_resource = models.DeploymentResource(properties=properties, sku=sku) return sdk_no_wait(no_wait, client.deployments.begin_update, - resource_group, service, name, deployment, deployment_resource) + resource_group, service, name, deployment.name, deployment_resource) -def app_get_build_log(cmd, client, resource_group, service, name, deployment=None): - if deployment is None: - deployment = client.apps.get( - resource_group, service, name).properties.active_deployment_name - if deployment is None: - raise CLIError(NO_PRODUCTION_DEPLOYMENT_ERROR) - deployment_properties = client.deployments.get( - resource_group, service, name, deployment).properties - if deployment_properties.source.type == "Jar" or deployment_properties.source.type == "NetCoreZip": - raise CLIError("{} deployment has no build logs.".format(deployment_properties.source.type)) - return stream_logs(client.deployments, resource_group, service, name, deployment) +def app_get_build_log(cmd, client, resource_group, service, name, deployment): + if deployment.properties.source.type != "Source": + raise CLIError("{} deployment has no build logs.".format(deployment.properties.source.type)) + return stream_logs(client.deployments, resource_group, service, name, deployment.name) def app_tail_log(cmd, client, resource_group, service, name, deployment=None, instance=None, follow=False, lines=50, since=None, limit=2048, format_json=None): if not instance: - if deployment is None: - deployment = client.apps.get( - resource_group, service, name).properties.active_deployment_name - if not deployment: - raise CLIError(NO_PRODUCTION_DEPLOYMENT_ERROR) - deployment_properties = client.deployments.get( - resource_group, service, name, deployment).properties - if not deployment_properties.instances: + if not deployment.properties.instances: raise CLIError("No instances found for deployment '{0}' in app '{1}'".format( - deployment, name)) - instances = deployment_properties.instances + deployment.name, name)) + instances = deployment.properties.instances if len(instances) > 1: logger.warning("Multiple app instances found:") for temp_instance in instances: @@ -1041,48 +978,24 @@ def deployment_list(cmd, client, resource_group, service, app): def deployment_generate_heap_dump(cmd, client, resource_group, service, app, app_instance, file_path, deployment=None): - if deployment is None: - logger.warning( - "No '--deployment' given, will update app's production deployment") - deployment = client.apps.get( - resource_group, service, app).properties.active_deployment_name - if deployment is None: - logger.warning("No production deployment found for update") - return diagnostic_parameters = models_20210901preview.DiagnosticParameters(app_instance=app_instance, file_path=file_path) logger.info("Heap dump is triggered.") - return client.deployments.begin_generate_heap_dump(resource_group, service, app, deployment, diagnostic_parameters) + return client.deployments.begin_generate_heap_dump(resource_group, service, app, deployment.name, diagnostic_parameters) def deployment_generate_thread_dump(cmd, client, resource_group, service, app, app_instance, file_path, deployment=None): - if deployment is None: - logger.warning( - "No '--deployment' given, will update app's production deployment") - deployment = client.apps.get( - resource_group, service, app).properties.active_deployment_name - if deployment is None: - logger.warning("No production deployment found for update") - return diagnostic_parameters = models_20210901preview.DiagnosticParameters(app_instance=app_instance, file_path=file_path) logger.info("Thread dump is triggered.") - return client.deployments.begin_generate_thread_dump(resource_group, service, app, deployment, diagnostic_parameters) + return client.deployments.begin_generate_thread_dump(resource_group, service, app, deployment.name, diagnostic_parameters) def deployment_start_jfr(cmd, client, resource_group, service, app, app_instance, file_path, duration=None, deployment=None): - if deployment is None: - logger.warning( - "No '--deployment' given, will update app's production deployment") - deployment = client.apps.get( - resource_group, service, app).properties.active_deployment_name - if deployment is None: - logger.warning("No production deployment found for update") - return diagnostic_parameters = models_20210901preview.DiagnosticParameters(app_instance=app_instance, file_path=file_path, duration=duration) logger.info("JFR is triggered.") - return client.deployments.begin_start_jfr(resource_group, service, app, deployment, diagnostic_parameters) + return client.deployments.begin_start_jfr(resource_group, service, app, deployment.name, diagnostic_parameters) def deployment_get(cmd, client, resource_group, service, app, name): diff --git a/src/spring-cloud/azext_spring_cloud/tests/latest/test_asc_app_validator.py b/src/spring-cloud/azext_spring_cloud/tests/latest/test_asc_app_validator.py new file mode 100644 index 00000000000..14b48d4fa25 --- /dev/null +++ b/src/spring-cloud/azext_spring_cloud/tests/latest/test_asc_app_validator.py @@ -0,0 +1,105 @@ +# -------------------------------------------------------------------------------------------- +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. See License.txt in the project root for license information. +# -------------------------------------------------------------------------------------------- +import unittest +import copy +from argparse import Namespace +from azure.cli.core.azclierror import InvalidArgumentValueError +from msrestazure.azure_exceptions import CloudError +from ..._app_validator import (fulfill_deployment_param) + +try: + import unittest.mock as mock +except ImportError: + from unittest import mock + +from azure.cli.core.mock import DummyCli +from azure.cli.core import AzCommandsLoader +from azure.cli.core.commands import AzCliCommand + + +def _get_test_cmd(): + cli_ctx = DummyCli() + cli_ctx.data['subscription_id'] = '00000000-0000-0000-0000-000000000000' + loader = AzCommandsLoader(cli_ctx, resource_type='Microsoft.AppPlatform') + cmd = AzCliCommand(loader, 'test', None) + cmd.command_kwargs = {'resource_type': 'Microsoft.AppPlatform'} + cmd.cli_ctx = cli_ctx + return cmd + + +def _get_deployment(resource_group, service, app, deployment, active): + resource = mock.MagicMock() + resource.id = '/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/{}/providers/Microsoft.AppPlatform/Spring/{}/apps/{}/deployments/{}'.format(resource_group, service, app, deployment) + resource.properties = mock.MagicMock() + resource.properties.active = active + return resource + + +class TestFulfillDeploymentParameter(unittest.TestCase): + @mock.patch('azext_spring_cloud._app_validator.cf_spring_cloud', autospec=True) + def test_deployment_provide(self, client_factory_mock): + client = mock.MagicMock() + client.deployments.get.return_value = _get_deployment('rg1', 'asc1', 'app1', 'green1', False) + client_factory_mock.return_value = client + + ns = Namespace(name='app1', service='asc1', resource_group='rg1', deployment='green1') + fulfill_deployment_param(_get_test_cmd(), ns) + self.assertEqual('/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/rg1/providers/Microsoft.AppPlatform/Spring/asc1/apps/app1/deployments/green1', + ns.deployment.id) + self.assertFalse(ns.deployment.properties.active) + + + @mock.patch('azext_spring_cloud._app_validator.cf_spring_cloud', autospec=True) + def test_deployment_provide_but_not_found(self, client_factory_mock): + client = mock.MagicMock() + resp = mock.MagicMock() + resp.status_code = 404 + resp.text = '{"Message": "Not Found"}' + client.deployments.get.side_effect = CloudError(resp, error='deployment not found.') + client_factory_mock.return_value = client + + ns = Namespace(name='app1', service='asc1', resource_group='rg1', deployment='green1') + with self.assertRaises(InvalidArgumentValueError) as context: + fulfill_deployment_param(_get_test_cmd(), ns) + self.assertEqual('Deployment green1 not found under app app1', str(context.exception)) + + @mock.patch('azext_spring_cloud._app_validator.cf_spring_cloud', autospec=True) + def test_deployment_with_active_deployment(self, client_factory_mock): + client = mock.MagicMock() + client.deployments.list.return_value = [ + _get_deployment('rg1', 'asc1', 'app1', 'green1', False), + _get_deployment('rg1', 'asc1', 'app1', 'default', True), + ] + client_factory_mock.return_value = client + + ns = Namespace(name='app1', service='asc1', resource_group='rg1', deployment=None) + fulfill_deployment_param(_get_test_cmd(), ns) + self.assertEqual('/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/rg1/providers/Microsoft.AppPlatform/Spring/asc1/apps/app1/deployments/default', + ns.deployment.id) + self.assertTrue(ns.deployment.properties.active) + + @mock.patch('azext_spring_cloud._app_validator.cf_spring_cloud', autospec=True) + def test_deployment_without_active_deployment(self, client_factory_mock): + client = mock.MagicMock() + client.deployments.list.return_value = [ + _get_deployment('rg1', 'asc1', 'app1', 'green1', False) + ] + client_factory_mock.return_value = client + + ns = Namespace(name='app1', service='asc1', resource_group='rg1', deployment=None) + with self.assertRaises(InvalidArgumentValueError) as context: + fulfill_deployment_param(_get_test_cmd(), ns) + self.assertEqual('No production deployment found, use --deployment to specify deployment or create deployment with: az spring-cloud app deployment create', str(context.exception)) + + @mock.patch('azext_spring_cloud._app_validator.cf_spring_cloud', autospec=True) + def test_deployment_without_deployment(self, client_factory_mock): + client = mock.MagicMock() + client.deployments.list.return_value = [] + client_factory_mock.return_value = client + + ns = Namespace(name='app1', service='asc1', resource_group='rg1', deployment=None) + with self.assertRaises(InvalidArgumentValueError) as context: + fulfill_deployment_param(_get_test_cmd(), ns) + self.assertEqual('No production deployment found, use --deployment to specify deployment or create deployment with: az spring-cloud app deployment create', str(context.exception)) \ No newline at end of file