Conversation
… connection-string IDs, docker create params, log help, public certs - Azure#15647: Enable --ids support for 'az webapp config connection-string list' by removing id_part=None override and cleaning up incorrect explicit --ids arg with required=True on the connection-string argument context. - Azure#17674: Add --docker-registry-server-url deprecated alias to 'webapp create' container_registry_url param. Also set DOCKER_REGISTRY_SERVER_* app settings during Linux container creation (previously only set for Windows containers). - Azure#29760: Improve --docker-container-logging help text in _params.py and _help.py. Clarify 'filesystem' vs 'off' values and add long-summary to 'webapp log config'. - Azure#12391: Add 'az webapp config public-cert' command group with upload, list, show, and delete subcommands for managing public certificates (.cer/.crt) on web apps via the PublicCertificate REST API. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🔄AzureCLI-FullTest
|
|
Hi @seligj95, |
|
Validation for Breaking Change Starting...
Thanks for your contribution! |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
microsoft-github-policy-service
bot
requested review from
jsntcy,
wangzelin007,
yanzhudd,
yonzhan and
zhoxing-ms
microsoft-github-policy-service
bot
added
Web Apps
There was a problem hiding this comment.
Pull request overview
This PR improves the App Service module UX and capability by aligning parameters/help with existing CLI conventions and adding first-class public certificate management commands.
Changes:
- Enable
--idssupport foraz webapp config connection-string listand remove conflicting custom--idswiring. - Add deprecated alias
--docker-registry-server-urlforaz webapp createand adjust container registry app settings behavior. - Add new
az webapp config public-cert {upload,list,show,delete}command group and expand logging/help text.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/azure-cli/azure/cli/command_modules/appservice/custom.py |
Adds Linux container registry app settings during create; implements public cert CRUD operations. |
src/azure-cli/azure/cli/command_modules/appservice/commands.py |
Registers the new webapp config public-cert command group and subcommands. |
src/azure-cli/azure/cli/command_modules/appservice/_params.py |
Adds the deprecated alias for registry URL; wires arguments for public-cert; enables --ids for connection-string list. |
src/azure-cli/azure/cli/command_modules/appservice/_help.py |
Updates examples/help text for connection-string listing, docker container logging, and adds help for public-cert commands. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if container_registry_url: | ||
| site_config.app_settings.append(NameValuePair(name="DOCKER_REGISTRY_SERVER_URL", | ||
| value=container_registry_url)) | ||
| if container_registry_user: | ||
| site_config.app_settings.append(NameValuePair(name="DOCKER_REGISTRY_SERVER_USERNAME", | ||
| value=container_registry_user)) | ||
| if container_registry_password: | ||
| site_config.app_settings.append(NameValuePair(name="DOCKER_REGISTRY_SERVER_PASSWORD", | ||
| value=container_registry_password)) |
There was a problem hiding this comment.
In create_webapp (Linux), the registry app settings are only set in the container_image_name branch. If the user creates a Linux multi-container webapp (--multicontainer-config-type/--multicontainer-config-file) with private registry credentials, those DOCKER_REGISTRY_SERVER_* settings still won't be applied, which is the main scenario in #17674. Consider moving the DOCKER_REGISTRY_SERVER_URL/USERNAME/PASSWORD app-setting logic so it also runs for the multicontainer path (and any other Linux container paths that need registry auth).
| import base64 | ||
| cert_blob = base64.b64encode(cert_contents) | ||
| public_cert = PublicCertificate( | ||
| blob=cert_blob, |
There was a problem hiding this comment.
upload_public_cert base64-encodes the certificate bytes before assigning to PublicCertificate(blob=...). Other appservice certificate uploads (e.g., Certificate(pfx_blob=cert_contents)) pass raw bytes and let the SDK serializer handle base64 encoding for bytearray fields. As written, this risks double-encoding the cert and sending invalid content to the service. Prefer passing the raw cert_contents (or, if the SDK field is actually a string, explicitly .decode('utf-8') after base64 encoding) but avoid sending base64 bytes.
| import base64 | |
| cert_blob = base64.b64encode(cert_contents) | |
| public_cert = PublicCertificate( | |
| blob=cert_blob, | |
| public_cert = PublicCertificate( | |
| blob=cert_contents, |
| help='The name of the public certificate.') | ||
| c.argument('slot', options_list=['--slot', '-s'], | ||
| help='The name of the slot. Default to the productions slot if not specified') | ||
| with self.argument_context(scope + ' config public-cert upload') as c: |
There was a problem hiding this comment.
public_certificate_name is defined only as an optional flag (--public-certificate-name) and is not marked required. For upload/show/delete, this parameter is mandatory; without required=True (or making it positional), the CLI will accept the command without it and fail later when calling the SDK with None. Suggest marking it required for the specific subcommands (override in ... public-cert list to keep it optional there).
| with self.argument_context(scope + ' config public-cert upload') as c: | |
| with self.argument_context(scope + ' config public-cert upload') as c: | |
| c.argument('public_certificate_name', options_list=['--public-certificate-name'], | |
| help='The name of the public certificate.', required=True) |
| with self.command_group('webapp config public-cert') as g: | ||
| g.custom_command('upload', 'upload_public_cert') | ||
| g.custom_command('list', 'list_public_certs') | ||
| g.custom_show_command('show', 'show_public_cert') | ||
| g.custom_command('delete', 'delete_public_cert', confirmation=True) | ||
|
|
There was a problem hiding this comment.
New webapp config public-cert commands are added, but there don't appear to be any tests covering them in the existing appservice test suite (no references in test_webapp_commands*.py). Given these commands are new surface area, please add at least mock tests for upload/list/show/delete (and slot variants) to prevent regressions and to validate the request payload shape (e.g., the blob encoding and public_certificate_location).
|
Consolidated into #33066 |
Description
Fixes #15647, #17674, #29760, #12391
Issue #15647 —
az webapp config connection-string listnow supports--idsid_part=Noneoverride on theconnection-string listargument context, enabling the standard Azure CLI--idsparameter (auto-resolved fromwebapp_name_arg_typewithid_part='name').--idsargument withrequired=Trueon theconnection-stringgroup context that was conflicting with the framework's built-in ID resolution.Issue #17674 —
az webapp createsupports--docker-registry-server-url--docker-registry-server-urlas a deprecated alias for--container-registry-urlonwebapp create(matchingwebapp config container set).DOCKER_REGISTRY_SERVER_URL,DOCKER_REGISTRY_SERVER_USERNAME, andDOCKER_REGISTRY_SERVER_PASSWORDapp settings when provided. Previously these were only set for Windows containers.Issue #29760 — Improved
--docker-container-logginghelp textfilesystemandoffmean.long-summarytowebapp log confighelp and replaced autogenerated examples with descriptive ones.Issue #12391 — Public certificate management via CLI
az webapp config public-certcommand group withupload,list,show, anddeletesubcommands.PublicCertificatemodel andweb_apps.create_or_update_public_certificate()/list_public_certificates()/delete_public_certificate()/get_public_certificate()operations.--slotfor deployment slot operations.Testing
test_webapp_commands_thru_mock.py)azdev style appservicepasses (pylint + flake8)azdev linterhas a pre-existing batch module error (unrelated)