[confcom] Add more thorough tests for `--with-containers` #9428

DomAyre · 2025-11-14T11:50:27Z

Why

The existing tests for --with-containers are okay but don't tests the full range of inputs we have in samples/.

How

Update the policy model to define which collections care about order for the sake of serialisation and comparison
Add for each sample, the reduced container definition that can be supplied to --with-containers to get policy.rego
Update the tests to check that parsing with --with-containers to produce policy.rego

This checklist is used to make sure that common guidelines for a pull request are followed.

Related command

General Guidelines

Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
Have you run python scripts/ci/test_index.py -q locally? (pip install wheel==0.30.0 required)
My extension version conforms to the Extension version schema

azure-client-tools-bot-prd · 2025-11-14T11:50:29Z

️✔️Azure CLI Extensions Breaking Change Test

️✔️Non Breaking Changes

yonzhan · 2025-11-14T11:50:37Z

Thank you for your contribution! We will review the pull request and get back to you soon.

github-actions · 2025-11-14T11:50:40Z

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).
After that please run the following commands to enable git hooks:

pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>

github-actions · 2025-11-14T11:54:21Z

For more info about extension versioning, please refer to Extension version schema

Copilot

Pull Request Overview

This PR enhances test coverage for the --with-containers flag by adding comprehensive tests for all samples and refactoring the policy model to use Pydantic dataclasses with explicit ordering semantics.

Key changes:

Migrated policy data model from standard Python dataclasses to Pydantic dataclasses with custom ordering support
Added container definition (.rego) files for each sample directory to support --with-containers testing
Implemented OPA (Open Policy Agent) integration for policy deserialization and comparison
Added parameterized test that validates policy generation from container definitions for all samples

Reviewed Changes

Copilot reviewed 29 out of 29 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
`setup.py`	Version bump to 1.4.2, added pydantic dependency, and OPA binary download during installation
`HISTORY.rst`	Added release notes for version 1.4.2 describing pydantic migration
`samples/aci//container.rego`	Added 20+ container definition files as test inputs for `--with-containers` testing
`azext_confcom/lib/policy.py`	Migrated from dataclasses to Pydantic with OrderlessField for collections where order is semantically irrelevant
`azext_confcom/lib/orderless_dataclasses.py`	New custom Pydantic dataclass wrapper that sorts orderless fields for serialization and comparison
`azext_confcom/lib/opa.py`	New OPA binary downloader and executor for policy evaluation
`azext_confcom/lib/binaries.py`	New utility for managing binary directory location
`azext_confcom/lib/serialization.py`	New policy serialization/deserialization utilities using OPA
`azext_confcom/security_policy.py`	Updated to use Pydantic's TypeAdapter for container serialization
`azext_confcom/tests/latest/test_confcom_acipolicygen_arm.py`	Added comprehensive parameterized test and renamed existing test for clarity

Copilot · 2025-11-14T14:49:37Z

src/confcom/azext_confcom/tests/latest/test_confcom_acipolicygen_arm.py

+
+@pytest.mark.parametrize(
+    "sample_directory",
+    os.listdir(SAMPLES_ROOT),


The test parameterizes over all directories in SAMPLES_ROOT without filtering. This means if any non-sample directories exist (e.g., .git, __pycache__, or other metadata directories), the test will fail when trying to open policy.rego which won't exist in those directories. Consider filtering the directories:

@pytest.mark.parametrize( "sample_directory", [d for d in os.listdir(SAMPLES_ROOT) if os.path.isdir(os.path.join(SAMPLES_ROOT, d)) and os.path.exists(os.path.join(SAMPLES_ROOT, d, "policy.rego"))], )

Suggested change

os.listdir(SAMPLES_ROOT),

[

d for d in os.listdir(SAMPLES_ROOT)

if os.path.isdir(os.path.join(SAMPLES_ROOT, d))

and os.path.exists(os.path.join(SAMPLES_ROOT, d, "policy.rego"))

],

Copilot · 2025-11-14T14:49:38Z

src/confcom/azext_confcom/lib/opa.py

+
+from azext_confcom.lib.binaries import get_binaries_dir
+
+_opa_pathh = os.path.abspath(os.path.join(get_binaries_dir(), "opa"))


Variable name typo: _opa_pathh has an extra 'h'. Should be _opa_path for consistency with naming conventions.

Copilot · 2025-11-14T14:49:38Z

src/confcom/azext_confcom/lib/opa.py

+_expected_sha256 = "fe8e191d44fec33db2a3d0ca788b9f83f866d980c5371063620c3c6822792877"
+
+
+def opa_get():
+
+    opa_fetch_resp = requests.get(
+        f"https://openpolicyagent.org/downloads/latest/opa_{platform.system().lower()}_amd64")
+    opa_fetch_resp.raise_for_status()
+
+    assert hashlib.sha256(opa_fetch_resp.content).hexdigest() == _expected_sha256
+
+    with open(_opa_pathh, "wb") as f:
+        f.write(opa_fetch_resp.content)
+


The OPA binary is downloaded during package installation without verifying the authenticity or checking if the file already exists. This could lead to:

Unnecessary network calls during every installation

No version pinning - the download URL uses "latest" which could break installations when new OPA versions are released

The hardcoded SHA256 hash will fail if OPA releases a new version

Consider:

Checking if the binary already exists before downloading

Pinning to a specific OPA version instead of "latest"

Updating the expected SHA256 hash when upgrading OPA versions

Suggested change

_expected_sha256 = "fe8e191d44fec33db2a3d0ca788b9f83f866d980c5371063620c3c6822792877"

def opa_get():

opa_fetch_resp = requests.get(

f"https://openpolicyagent.org/downloads/latest/opa_{platform.system().lower()}_amd64")

opa_fetch_resp.raise_for_status()

assert hashlib.sha256(opa_fetch_resp.content).hexdigest() == _expected_sha256

with open(_opa_pathh, "wb") as f:

f.write(opa_fetch_resp.content)

_OPA_VERSION = "v0.63.0"

_expected_sha256 = "fe8e191d44fec33db2a3d0ca788b9f83f866d980c5371063620c3c6822792877"

def opa_get():

# Check if OPA binary exists and matches expected hash

if os.path.isfile(_opa_pathh):

with open(_opa_pathh, "rb") as f:

file_hash = hashlib.sha256(f.read()).hexdigest()

if file_hash == _expected_sha256:

return _opa_pathh

# Download OPA binary if not present or hash mismatch

url = f"https://openpolicyagent.org/downloads/{_OPA_VERSION}/opa_{platform.system().lower()}_amd64"

opa_fetch_resp = requests.get(url)

opa_fetch_resp.raise_for_status()

assert hashlib.sha256(opa_fetch_resp.content).hexdigest() == _expected_sha256, "Downloaded OPA binary hash mismatch"

with open(_opa_pathh, "wb") as f:

f.write(opa_fetch_resp.content)

Copilot · 2025-11-14T14:49:38Z

src/confcom/setup.py

 SecurityPolicyProxy.download_binaries()
 KataPolicyGenProxy.download_binaries()
 CoseSignToolProxy.download_binaries()
+opa_get()


The opa_get() function is called during package setup without error handling. If the download fails (network issues, URL unavailable, etc.), the package installation will fail. Consider:

Adding a try-except block around the call in setup.py

Checking if the binary already exists before attempting download

Providing a fallback mechanism or clearer error message for users

Example:

try: if not os.path.exists(os.path.join(get_binaries_dir(), "opa")): opa_get() except Exception as e: logger.warn(f"Failed to download OPA binary: {e}. You may need to download it manually.")

Copilot · 2025-11-14T14:49:38Z

src/confcom/samples/aci/environment_variables/container1.rego

+            "pattern":"MY_VAR=MY_VAL",
+            "required":false,
+            "strategy":"string"


Missing space in formatting - there's no space after the colon in the JSON field. This creates inconsistent formatting:

"pattern":"MY_VAR=MY_VAL",

Should be:

"pattern": "MY_VAR=MY_VAL",

This is inconsistent with the formatting in the rest of the file (see lines 9-11 which have proper spacing).

Suggested change

"pattern":"MY_VAR=MY_VAL",

"required":false,

"strategy":"string"

"pattern": "MY_VAR=MY_VAL",

"required": false,

"strategy": "string"

Copilot · 2025-11-14T14:49:39Z

src/confcom/azext_confcom/tests/latest/test_confcom_acipolicygen_arm.py

+            with open(os.path.join(SAMPLES_ROOT, sample_directory, file_path)) as input_containers_file:
+                input_containers.append(json.loads(input_containers_file.read()))


File is opened without a context manager. Use a with statement for proper resource management:

with open(os.path.join(SAMPLES_ROOT, sample_directory, file_path)) as input_containers_file: input_containers.append(json.loads(input_containers_file.read()))

Copilot · 2025-11-14T14:49:39Z

src/confcom/azext_confcom/tests/latest/test_confcom_acipolicygen_arm.py

+    # Load input container definition
+    input_containers = []
+    for file_path in os.listdir(os.path.join(SAMPLES_ROOT, sample_directory)):
+        if file_path.startswith("container"):


The loop that collects container definition files assumes all files starting with "container" are valid JSON container definitions. This could cause issues if there are other files (like container.md, backup files like container1.rego~, etc.). Consider being more specific with the file matching:

for file_path in os.listdir(os.path.join(SAMPLES_ROOT, sample_directory)): if file_path.startswith("container") and file_path.endswith(".rego"): with open(os.path.join(SAMPLES_ROOT, sample_directory, file_path)) as input_containers_file: input_containers.append(json.loads(input_containers_file.read()))

Suggested change

if file_path.startswith("container"):

if file_path.startswith("container") and file_path.endswith(".json"):

Copilot · 2025-11-14T14:49:39Z

src/confcom/azext_confcom/lib/opa.py

+        f"https://openpolicyagent.org/downloads/latest/opa_{platform.system().lower()}_amd64")
+    opa_fetch_resp.raise_for_status()
+
+    assert hashlib.sha256(opa_fetch_resp.content).hexdigest() == _expected_sha256


The assertion will cause a cryptic error message if the SHA256 hash doesn't match. Consider using a more informative error message:

actual_hash = hashlib.sha256(opa_fetch_resp.content).hexdigest() if actual_hash != _expected_sha256: raise ValueError(f"OPA binary hash mismatch. Expected {_expected_sha256}, got {actual_hash}")

Suggested change

assert hashlib.sha256(opa_fetch_resp.content).hexdigest() == _expected_sha256

actual_hash = hashlib.sha256(opa_fetch_resp.content).hexdigest()

if actual_hash != _expected_sha256:

raise ValueError(f"OPA binary hash mismatch. Expected {_expected_sha256}, got {actual_hash}")

Copilot · 2025-11-14T14:49:40Z

src/confcom/azext_confcom/tests/latest/test_confcom_acipolicygen_arm.py

+    with open(os.path.join(SAMPLES_ROOT, sample_directory, "policy.rego")) as expected_policy_file:
+        expected_policy_str = expected_policy_file.read()


The test opens and reads a file without a context manager. If an exception occurs, the file handle may not be properly closed. Use a with statement for proper resource management:

with open(os.path.join(SAMPLES_ROOT, sample_directory, "policy.rego")) as expected_policy_file: expected_policy_str = expected_policy_file.read()

Copilot · 2025-11-14T14:49:40Z

src/confcom/azext_confcom/lib/opa.py

+def opa_get():
+
+    opa_fetch_resp = requests.get(
+        f"https://openpolicyagent.org/downloads/latest/opa_{platform.system().lower()}_amd64")


The opa_get() function downloads a binary from the internet without verifying HTTPS certificate or using a secure connection verification. The requests.get() call should explicitly verify SSL certificates to prevent man-in-the-middle attacks:

opa_fetch_resp = requests.get( f"https://openpolicyagent.org/downloads/latest/opa_{platform.system().lower()}_amd64", verify=True, # Explicitly verify SSL certificates (default behavior, but explicit is better) timeout=30 # Add timeout to prevent hanging )

Additionally, there's no timeout which could cause the installation to hang indefinitely if the server doesn't respond.

Suggested change

f"https://openpolicyagent.org/downloads/latest/opa_{platform.system().lower()}_amd64")

f"https://openpolicyagent.org/downloads/latest/opa_{platform.system().lower()}_amd64",

verify=True, # Explicitly verify SSL certificates

timeout=30 # Add timeout to prevent hanging

)

DomAyre added 4 commits November 12, 2025 13:36

Define which policy fields care about ordering

562139c

Prep for PR

850c23c

Merge branch 'main' into with-containers-tests

205bdc7

Update version and history

719e669

azure-client-tools-bot-prd bot added this to the December 2025 (2025-12-02) milestone Nov 14, 2025

DomAyre added 4 commits November 14, 2025 12:12

Fix style checks

19286d6

Attempt to fix style not flagged locally

02dc5fe

Add missing opa_get command to setup.py

68ad74e

Update expected sha for opa

ce747e1

DomAyre marked this pull request as ready for review November 14, 2025 14:37

Copilot AI review requested due to automatic review settings November 14, 2025 14:37

Copilot started reviewing on behalf of DomAyre November 14, 2025 14:38 View session

Copilot finished reviewing on behalf of DomAyre November 14, 2025 14:40

Copilot AI reviewed Nov 14, 2025

View reviewed changes

DomAyre mentioned this pull request Nov 14, 2025

Support generating container descriptions from radius app descriptions DomAyre/azure-cli-extensions#9

Closed

13 tasks

yonzhan requested review from jsntcy and kairu-ms November 14, 2025 23:20

yonzhan assigned kairu-ms Nov 14, 2025

yonzhan requested a review from wangzelin007 November 14, 2025 23:20

-    os.listdir(SAMPLES_ROOT),
+    [
+        d for d in os.listdir(SAMPLES_ROOT)
+        if os.path.isdir(os.path.join(SAMPLES_ROOT, d))
+        and os.path.exists(os.path.join(SAMPLES_ROOT, d, "policy.rego"))
+    ],


		from azext_confcom.lib.binaries import get_binaries_dir

		_opa_pathh = os.path.abspath(os.path.join(get_binaries_dir(), "opa"))

		with open(os.path.join(SAMPLES_ROOT, sample_directory, file_path)) as input_containers_file:
		input_containers.append(json.loads(input_containers_file.read()))

	if file_path.startswith("container"):
	if file_path.startswith("container") and file_path.endswith(".json"):

-    assert hashlib.sha256(opa_fetch_resp.content).hexdigest() == _expected_sha256
+    actual_hash = hashlib.sha256(opa_fetch_resp.content).hexdigest()
+    if actual_hash != _expected_sha256:
+        raise ValueError(f"OPA binary hash mismatch. Expected {_expected_sha256}, got {actual_hash}")

		with open(os.path.join(SAMPLES_ROOT, sample_directory, "policy.rego")) as expected_policy_file:
		expected_policy_str = expected_policy_file.read()

-        f"https://openpolicyagent.org/downloads/latest/opa_{platform.system().lower()}_amd64")
+        f"https://openpolicyagent.org/downloads/latest/opa_{platform.system().lower()}_amd64",
+        verify=True,  # Explicitly verify SSL certificates
+        timeout=30    # Add timeout to prevent hanging
+    )

[confcom] Add more thorough tests for --with-containers #9428

Are you sure you want to change the base?

[confcom] Add more thorough tests for --with-containers #9428

Uh oh!

Conversation

DomAyre commented Nov 14, 2025

Why

How

Related command

General Guidelines

Uh oh!

azure-client-tools-bot-prd bot commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yonzhan commented Nov 14, 2025

Uh oh!

github-actions bot commented Nov 14, 2025

Uh oh!

github-actions bot commented Nov 14, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[confcom] Add more thorough tests for `--with-containers` #9428

[confcom] Add more thorough tests for `--with-containers` #9428

azure-client-tools-bot-prd bot commented Nov 14, 2025 •

edited

Loading