Support tls feature in api version 2021-09-01-preview #3976
Conversation
| @@ -102,12 +104,13 @@ def load_command_table(self, _): | |||
| g.custom_command('redis update', 'binding_redis_update') | |||
| g.custom_show_command('remove', 'binding_remove') | |||
|
|
|||
| with self.command_group('spring-cloud certificate', client_factory=cf_spring_cloud, | |||
| with self.command_group('spring-cloud certificate', client_factory=cf_spring_cloud_20210901preview, | |||
There was a problem hiding this comment.
Should we just set the default API version to 2021-09-01-preview?
There was a problem hiding this comment.
If not, we would update all clients related with certificate to 2021-09-01-preview one by one.
| assign_identity=None, | ||
| loaded_public_cert_file=None): | ||
|
|
||
| client = get_mgmt_service_client(cmd.cli_ctx, AppPlatformManagementClient_20210901preview) |
There was a problem hiding this comment.
Same as above, change the default client to new API version?
There was a problem hiding this comment.
For app, maybe change only certificate related to new api version is better
| certificate_resource = models.CertificateResource(properties=properties) | ||
| def certificate_add(cmd, client, resource_group, service, name, only_public_cert=None, | ||
| vault_uri=None, vault_certificate_name=None, public_cert_file=None): | ||
| if vault_uri is None and public_cert_file is None: |
There was a problem hiding this comment.
Move such logic to validators?
There was a problem hiding this comment.
Seems validator could only validate one parameter? And here we need to validate between parameters
| if load_certificate.resource_id == certificate_resource_id: | ||
| reference_apps.append(app) | ||
| break | ||
| for app in reference_apps: |
There was a problem hiding this comment.
Why do we need this change?
There was a problem hiding this comment.
Add deployment info or not is both ok, I add here because currently app get/list all have deployment information
|
spring cloud |
Descatles
changed the title
src/spring-cloud/HISTORY.md
Outdated
| * Application could load public certificate by using argument`--loaded_public_certificate_file` in batch or | ||
| directly using `spring-cloud app append-loaded-public-certificate` one by one |
There was a problem hiding this comment.
| * Application could load public certificate by using argument`--loaded_public_certificate_file` in batch or | |
| directly using `spring-cloud app append-loaded-public-certificate` one by one | |
| * Application could load public certificate by using argument `--loaded_public_certificate_file` in batch or | |
| directly using `spring-cloud app append-loaded-public-certificate` one by one |
| raise CLIError("loadedCertificates must be provided in the json file") | ||
| loaded_certificates = [] | ||
| for item in data['loadedCertificates']: | ||
| invalidProperties = not item.get('certificateName') or not item.get('loadTrustStore') | ||
| if invalidProperties: | ||
| raise CLIError("certificateName, loadTrustStore must be provided in the json file") |
There was a problem hiding this comment.
Could you please use the specific error type from azure.cli.core.azclierror instead of CLIError
| if not data.get('loadedCertificates'): | ||
| raise CLIError("loadedCertificates must be provided in the json file") | ||
| loaded_certificates = [] | ||
| for item in data['loadedCertificates']: | ||
| invalidProperties = not item.get('certificateName') or not item.get('loadTrustStore') | ||
| if invalidProperties: | ||
| raise CLIError("certificateName, loadTrustStore must be provided in the json file") |
There was a problem hiding this comment.
Could you please use the specific error type from azure.cli.core.azclierror instead of CLIError
| raise CLIError( | ||
| "To use the log streaming feature, please enable the test endpoint by running 'az spring-cloud test-endpoint enable -n {0} -g {1}'".format( | ||
| service, resource_group)) |
There was a problem hiding this comment.
Same as above, I won't comment repeatedly for the similar palaces
| @@ -64,3 +64,36 @@ def test_bind_cert_to_domain(self): | |||
|
|
|||
| self.cmd('spring-cloud certificate remove --name {cert} -g {rg} -s {serviceName}') | |||
| self.cmd('spring-cloud certificate show --name {cert} -g {rg} -s {serviceName}', expect_failure=True) | |||
|
|
|||
|
|
|||
| @record_only() | |||
There was a problem hiding this comment.
May I ask why this test is @record_only()?
There was a problem hiding this comment.
Actually I don't know its meaning, I add that just because the former test class in this file add that. Is that necessary?
There was a problem hiding this comment.
Please refer to this document for details: https://github.com/Azure/azure-cli/blob/dev/doc/authoring_tests.md#running-tests-live
This checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines
azdev style <YOUR_EXT>locally? (pip install azdevrequired)python scripts/ci/test_index.py -qlocally?For new extensions:
About Extension Publish
There is a pipeline to automatically build, upload and publish extension wheels.
Once your PR is merged into master branch, a new PR will be created to update
src/index.jsonautomatically.The precondition is to put your code inside this repo and upgrade the version in the PR but do not modify
src/index.json.