App Service] az webapp sitecontainers convert: Update to convert COMP…#33131
Open
kumaramit-msft wants to merge 2 commits intoAzure:devfrom
Open
App Service] az webapp sitecontainers convert: Update to convert COMP…#33131kumaramit-msft wants to merge 2 commits intoAzure:devfrom
kumaramit-msft wants to merge 2 commits intoAzure:devfrom
Conversation
…OSE to Sitecontainers
kumaramit-msft
requested review from
NoriZC,
teresaritorto and
yanzhudd
as code owners
❌AzureCLI-FullTest
|
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| webapp sitecontainers convert | cmd webapp sitecontainers convert added parameter main_container_name |
Collaborator
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
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>
|
microsoft-github-policy-service
bot
requested review from
Greedygre,
JennyLawrance,
Juliehzl,
chinadragon0515,
ebencarek,
howang-ms,
jijohn14,
jsntcy,
pagariyaalok,
ruslany,
sanchitmehta,
torosent,
vinisoto,
vturecek,
wangzelin007,
yonzhan and
zhoxing-ms
Contributor
There was a problem hiding this comment.
Pull request overview
Adds COMPOSE (linuxFxVersion=COMPOSE|<base64>) support to az webapp sitecontainers convert --mode sitecontainers, enabling conversion of multi-container Docker Compose webapps into the Sitecontainers model, plus a large suite of unit tests and YAML fixtures to validate parsing and orchestration behavior.
Changes:
- Extend
convert_webapp_sitecontainersto convert both DOCKER and COMPOSE apps to sitecontainers (including--main-container-name, warnings, and rollback-on-failure behavior). - Introduce Compose parsing/mapping helpers for environment variables, ports, volumes, entrypoint/command →
startUpCommand, and service name sanitization. - Add comprehensive unit tests + sample Compose YAML fixtures covering supported/unsupported scenarios.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/azure-cli/azure/cli/command_modules/appservice/custom.py | Implements COMPOSE→sitecontainers conversion flow, parsing helpers, and production-site warning prompt. |
| src/azure-cli/azure/cli/command_modules/appservice/_params.py | Adds --main-container-name parameter for conversion command. |
| src/azure-cli/azure/cli/command_modules/appservice/_help.py | Updates help text/examples to mention COMPOSE support and main-container selection. |
| src/azure-cli/azure/cli/command_modules/appservice/tests/latest/test_compose_convert.py | Adds unit tests for helper parsing and conversion orchestration (mocked ARM calls). |
| src/azure-cli/azure/cli/command_modules/appservice/tests/latest/compose-convert-basic.yml | Fixture: basic multi-container compose. |
| src/azure-cli/azure/cli/command_modules/appservice/tests/latest/compose-convert-env-mapping.yml | Fixture: env mapping format. |
| src/azure-cli/azure/cli/command_modules/appservice/tests/latest/compose-convert-env-sequence.yml | Fixture: env sequence format. |
| src/azure-cli/azure/cli/command_modules/appservice/tests/latest/compose-convert-entrypoint-command.yml | Fixture: entrypoint/command variants. |
| src/azure-cli/azure/cli/command_modules/appservice/tests/latest/compose-convert-volumes-bind.yml | Fixture: ${WEBAPP_STORAGE_HOME} bind mounts. |
| src/azure-cli/azure/cli/command_modules/appservice/tests/latest/compose-convert-volumes-long.yml | Fixture: long-syntax volumes. |
| src/azure-cli/azure/cli/command_modules/appservice/tests/latest/compose-convert-volumes-named.yml | Fixture: named volumes. |
| src/azure-cli/azure/cli/command_modules/appservice/tests/latest/compose-convert-unsupported-bind.yml | Fixture: unsupported host/relative bind mounts. |
| src/azure-cli/azure/cli/command_modules/appservice/tests/latest/compose-convert-port-conflict.yml | Fixture: port conflict scenario. |
| src/azure-cli/azure/cli/command_modules/appservice/tests/latest/compose-convert-no-ports.yml | Fixture: no ports → main-container fallback. |
| src/azure-cli/azure/cli/command_modules/appservice/tests/latest/compose-convert-multi-port.yml | Fixture: multiple ports per service. |
| src/azure-cli/azure/cli/command_modules/appservice/tests/latest/compose-convert-full.yml | Fixture: full realistic multi-service compose. |
| src/azure-cli/azure/cli/command_modules/appservice/tests/latest/compose-convert-underscore-names.yml | Fixture: service name sanitization cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| url = ( | ||
| f"/subscriptions/{subscription_id}/resourceGroups/{resource_group}/" | ||
| f"providers/Microsoft.Web/sites/{name}{slot_segment}/config/appsettings/list?api-version=2023-12-01" | ||
| f"providers/Microsoft.Web/sites/{name}{slot_segment}/config/appsettings/list?api-version=2024-11-01" |
There was a problem hiding this comment.
The app settings list ARM call hard-codes api-version=2024-11-01. This is a compatibility risk (e.g., older clouds / API profiles) and it’s also the only place in this module using that version. Consider reverting to the previously used API version for this endpoint or using the existing get_app_settings(...) / _get_site_props_for_sitecontainer_app_internal(...) helpers instead of a raw request.
Suggested change
| f"providers/Microsoft.Web/sites/{name}{slot_segment}/config/appsettings/list?api-version=2024-11-01" | |
| f"providers/Microsoft.Web/sites/{name}{slot_segment}/config/appsettings/list" | |
| f"?api-version={VERSION_2022_09_01}" |
| ComposeFileParser.ParsePorts). Returns a list of tuples. | ||
| """ | ||
| if ports_node is None: | ||
| return [] |
There was a problem hiding this comment.
_parse_compose_ports assumes ports_node is iterable and will iterate over characters if a scalar string is provided (e.g. ports: "80:80"). Add a type check similar to the env/volumes parsers (accept list, optionally accept scalar string by wrapping it in a list) to avoid silently mis-parsing ports.
Suggested change
| return [] | |
| return [] | |
| if isinstance(ports_node, str): | |
| ports_node = [ports_node] | |
| elif not isinstance(ports_node, list): | |
| logger.warning(" [ports] Unexpected ports format (not list or string). Skipping.") | |
| return [] |
Comment on lines
+3136
to
+3162
| # Create the SiteContainer directly (not via create_webapp_sitecontainers) | ||
| # because environment_variables and volume_mounts are not exposed as | ||
| # individual kwargs on the higher-level create function. | ||
| auth_type = AuthType.ANONYMOUS | ||
| if auth_kwargs.get("system_assigned_identity"): | ||
| auth_type = AuthType.SYSTEM_IDENTITY | ||
| elif auth_kwargs.get("user_assigned_identity"): | ||
| auth_type = AuthType.USER_ASSIGNED | ||
| elif auth_kwargs.get("registry_username") and auth_kwargs.get("registry_password"): | ||
| auth_type = AuthType.USER_CREDENTIALS | ||
|
|
||
| sitecontainer = SiteContainer( | ||
| image=spec["image"], | ||
| target_port=spec["target_port"], | ||
| start_up_command=spec["startup_command"], | ||
| is_main=is_main, | ||
| auth_type=auth_type, | ||
| user_name=auth_kwargs.get("registry_username"), | ||
| password_secret=auth_kwargs.get("registry_password"), | ||
| user_managed_identity_client_id=auth_kwargs.get("user_assigned_identity"), | ||
| volume_mounts=spec["volume_mounts"], | ||
| environment_variables=spec["env_variables"], | ||
| # Non-main (sidecar) containers should NOT inherit the webapp's | ||
| # app settings and connection strings by default. They receive | ||
| # only the env vars explicitly declared in the compose file. | ||
| inherit_app_settings_and_connection_strings=None if is_main else False, | ||
| ) |
There was a problem hiding this comment.
The COMPOSE conversion path calls _create_or_update_webapp_sitecontainer_internal(...) directly, which bypasses _validate_sitecontainer_internal(...) (targetPort range/uniqueness, env var app-setting reference validation, and managed identity enablement checks). Consider reusing the same validation pipeline as create_webapp_sitecontainers(...) (e.g., call _get_site_props_for_sitecontainer_app_internal(...) once and validate each SiteContainerSpec before making ARM create calls) so failures are caught early and errors are more actionable.
Comment on lines
+3105
to
+3107
| # ----------------------------------------------------------------------- | ||
| if new_app_settings: | ||
| logger.warning("Creating %d app setting(s) for environment variable references...", len(new_app_settings)) |
There was a problem hiding this comment.
App settings are updated (Step 6) before any sitecontainer resources are created, but the rollback logic on failure only deletes created containers. If a later container create fails, the conversion leaves behind newly added/modified COMPOSE_... app settings. Consider either (a) applying app settings after all container validation passes and/or (b) tracking previous values and reverting the changes on rollback.
Suggested change
| # ----------------------------------------------------------------------- | |
| if new_app_settings: | |
| logger.warning("Creating %d app setting(s) for environment variable references...", len(new_app_settings)) | |
| # ----------------------------------------------------------------------- | |
| previous_new_app_settings = {} | |
| if new_app_settings: | |
| logger.warning("Creating %d app setting(s) for environment variable references...", len(new_app_settings)) | |
| client = web_client_factory(cmd.cli_ctx) | |
| if slot: | |
| current_app_settings = client.web_apps.list_application_settings_slot( | |
| resource_group_name=resource_group, | |
| name=name, | |
| slot=slot | |
| ).properties or {} | |
| else: | |
| current_app_settings = client.web_apps.list_application_settings( | |
| resource_group_name=resource_group, | |
| name=name | |
| ).properties or {} | |
| previous_new_app_settings = { | |
| key: current_app_settings[key] | |
| for key in new_app_settings | |
| if key in current_app_settings | |
| } |
| import os | ||
| import unittest | ||
| from base64 import b64encode | ||
| from unittest.mock import MagicMock, patch, call |
There was a problem hiding this comment.
call is imported but never used in this test module. This can trip pylint/flake8 unused-import checks; please remove it.
Suggested change
| from unittest.mock import MagicMock, patch, call | |
| from unittest.mock import MagicMock, patch |
| _make_named_volume_mount, | ||
| _sanitize_container_name, | ||
| _convert_compose_to_sitecontainers, | ||
| _COMPOSE_WEBAPP_STORAGE_HOME, |
There was a problem hiding this comment.
_COMPOSE_WEBAPP_STORAGE_HOME is imported from custom but not referenced anywhere in this test file, which is likely to fail unused-import linting. Please remove it or add a test that exercises/uses it.
Suggested change
| _COMPOSE_WEBAPP_STORAGE_HOME, |
Comment on lines
+13
to
+20
| API_KEY: "s3cret-key-value" | ||
| db: | ||
| image: "postgres:15-alpine" | ||
| ports: | ||
| - "5432:5432" | ||
| environment: | ||
| POSTGRES_USER: admin | ||
| POSTGRES_PASSWORD: "p@ssw0rd" |
There was a problem hiding this comment.
These fixtures include password/API-key-like literal values (e.g., API_KEY, POSTGRES_PASSWORD) which can trigger repository secret scanning and fail CI. Consider replacing with clearly fake placeholders (e.g., not-a-real-password) or otherwise aligning with the repo’s secret-scan suppression approach for test data.
Suggested change
| API_KEY: "s3cret-key-value" | |
| db: | |
| image: "postgres:15-alpine" | |
| ports: | |
| - "5432:5432" | |
| environment: | |
| POSTGRES_USER: admin | |
| POSTGRES_PASSWORD: "p@ssw0rd" | |
| API_KEY: "not-a-real-api-key" | |
| db: | |
| image: "postgres:15-alpine" | |
| ports: | |
| - "5432:5432" | |
| environment: | |
| POSTGRES_USER: admin | |
| POSTGRES_PASSWORD: "not-a-real-password" |
Comment on lines
+26
to
+47
| environment: | ||
| WORDPRESS_DB_HOST: "localhost:3306" | ||
| WORDPRESS_DB_USER: wp_user | ||
| WORDPRESS_DB_PASSWORD: "wp_s3cret" | ||
| WORDPRESS_DB_NAME: wordpress | ||
| WORDPRESS_TABLE_PREFIX: wp_ | ||
| WORDPRESS_CONFIG_EXTRA: | | ||
| define('WP_REDIS_HOST', 'localhost'); | ||
| define('WP_REDIS_PORT', 6379); | ||
| restart: always | ||
|
|
||
| mariadb: | ||
| image: "mariadb:11" | ||
| ports: | ||
| - "3306:3306" | ||
| volumes: | ||
| - "${WEBAPP_STORAGE_HOME}/mysql/data:/var/lib/mysql" | ||
| environment: | ||
| - MYSQL_ROOT_PASSWORD=rootpass123 | ||
| - MYSQL_DATABASE=wordpress | ||
| - MYSQL_USER=wp_user | ||
| - MYSQL_PASSWORD=wp_s3cret |
There was a problem hiding this comment.
This sample compose file contains password-like values (e.g., WORDPRESS_DB_PASSWORD, MYSQL_ROOT_PASSWORD, MYSQL_PASSWORD). These strings may be flagged by automated secret scanners in CI. Consider swapping them to obvious non-secret placeholders to avoid false positives in security tooling.
| - "80:80" | ||
| environment: | ||
| - REDIS_URL=redis://localhost:6379 | ||
| - APP_SECRET=mysecretvalue |
There was a problem hiding this comment.
APP_SECRET=mysecretvalue and similar literals in fixtures can trigger secret scanning (even though they’re test-only). Consider using clearly fake placeholders that won’t match common secret patterns.
Suggested change
| - APP_SECRET=mysecretvalue | |
| - APP_SECRET=not-a-real-secret |
| volumes: | ||
| - "${WEBAPP_STORAGE_HOME}/mysql/data:/var/lib/mysql" | ||
| environment: | ||
| MYSQL_ROOT_PASSWORD: rootpass |
There was a problem hiding this comment.
MYSQL_ROOT_PASSWORD: rootpass looks like a real secret and may be flagged by CI secret scanning despite being test data. Consider replacing with an explicit placeholder value (e.g., example-password) that won’t match common password heuristics.
Suggested change
| MYSQL_ROOT_PASSWORD: rootpass | |
| MYSQL_ROOT_PASSWORD: example-password |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[App Service]
az webapp sitecontainers convert: Add COMPOSE to Sitecontainers conversionRelated command
az webapp sitecontainers convert --mode sitecontainersDescription
This PR adds support for converting multi-container Docker Compose apps (
COMPOSE|<base64>linuxFxVersion) to the Sitecontainers model. The conversion:linuxFxVersion--main-container-nameCOMPOSE_<SERVICE>_<VAR>app settings for inline environment variables${WEBAPP_STORAGE_HOME}bind mounts to/homevolumeSubPath, and named volumes to ephemeral local storagelinuxFxVersiontoSITECONTAINERSon success--slot(sitecontainers mode only)Also includes:
Testing Guide
History Notes
[App Service]
az webapp sitecontainers convert: Add support for converting Docker Compose multi-container apps to Sitecontainers modeThis checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.