Conversation
️✔️Azure CLI Extensions Breaking Change Test
|
|
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. |
There was a problem hiding this comment.
Pull request overview
This PR reverts the 1.5.0 release and rolls back to version 1.4.5 by undoing the changes introduced in PR #9477.
- Reverts version from 1.5.0 to 1.4.5 in setup.py
- Removes the
confcom fragmentcommand group and associated functionality (fragment pushandfragment attachcommands) - Reverts certificate generation script changes to no longer accept an output path parameter
- Reverts test infrastructure changes including subprocess invocation methods and file path handling
- Removes the genpolicy-settings.json file from source (it will be downloaded during installation)
- Reverts policy rules and validation changes
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/confcom/setup.py | Version reverted from 1.5.0 to 1.4.5 |
| src/confcom/samples/certs/create_certchain.sh | Script no longer accepts output path parameter, uses RootPath throughout |
| src/confcom/azext_confcom/tests/latest/test_confcom_virtual_node.py | Reverted path handling and subprocess call changes |
| src/confcom/azext_confcom/tests/latest/test_confcom_tar.py | Reverted test file output location |
| src/confcom/azext_confcom/tests/latest/test_confcom_fragment.py | Reverted path handling and subprocess call changes |
| src/confcom/azext_confcom/tests/latest/test_confcom_arm.py | Removed complex Docker cleanup coordination logic |
| src/confcom/azext_confcom/tests/latest/test_confcom_acifragmentgen.py | Deleted entire test file for fragment generation |
| src/confcom/azext_confcom/data/rules.rego | Removed BUNDLE_ID constant, inlined bundle ID regex |
| src/confcom/azext_confcom/data/genpolicy-settings.json | Deleted file (will be downloaded during setup) |
| src/confcom/azext_confcom/custom.py | Removed fragment command wrappers and out_signed_fragment parameter |
| src/confcom/azext_confcom/commands.py | Removed confcom fragment command group |
| src/confcom/azext_confcom/command/fragment_push.py | Deleted file containing fragment push functionality |
| src/confcom/azext_confcom/command/fragment_attach.py | Deleted file containing fragment attach functionality |
| src/confcom/azext_confcom/_params.py | Removed parameters for fragment commands |
| src/confcom/azext_confcom/_help.py | Removed help text for fragment commands |
| src/confcom/HISTORY.rst | Removed 1.5.0 release notes |
| linter_exclusions.yml | Removed linter exclusions for fragment commands |
Comments suppressed due to low confidence (1)
src/confcom/azext_confcom/data/genpolicy-settings.json:1
- The file
genpolicy-settings.jsonis referenced inkata_proxy.pyat lines 38, 39, and 129, but this revert deletes the file. This will cause runtime errors when the katapolicygen functionality tries to use the default settings file.
Either:
- The file should not be deleted if it's still needed, or
- The references in
kata_proxy.pyneed to be updated to point to a different location or download source
The code expects the file to be downloaded from a GitHub release URL, so it may work if the download mechanism functions properly. However, verify that the remote file at the specified URL exists and is accessible.
| with tempfile.TemporaryDirectory() as folder: | ||
| filename = os.path.join(folder, "oci.tar") | ||
| filename2 = os.path.join(folder, "oci2.tar") | ||
| filename2 = os.path.join(self.path, "oci2.tar") |
There was a problem hiding this comment.
Writing the tar file to self.path (the test directory) instead of the temporary directory will create test artifacts in the repository directory. This can cause issues with version control, test isolation, and cleanup. The tar file oci2.tar will persist after the test completes.
Consider using the temporary directory for all test artifacts to ensure proper cleanup and test isolation.
| if upload_fragment and image_target: | ||
| oras_proxy.attach_fragment_to_image(image_target, out_path) | ||
| elif upload_fragment: | ||
| oras_proxy.push_fragment_to_registry(feed, out_path) |
There was a problem hiding this comment.
This revert changes the behavior of the upload_fragment flag. In version 1.5.0, when upload_fragment was True and no image_target was specified, the fragment would be attached to the first image in the input (policy_images[0]). After this revert, if no image_target is provided, the fragment will be pushed as a standalone fragment to the registry using feed instead.
This behavioral change may affect existing users who relied on the automatic attachment to the first image. Consider documenting this change in release notes or migration guides to help users understand the different behavior between versions.
| arg_list, | ||
| check=False, | ||
| shell=False, | ||
| shell=True, |
There was a problem hiding this comment.
The change from shell=False to shell=True for the subprocess call introduces a potential security vulnerability. When shell=True, the command is executed through the shell, which can be dangerous if any arguments are derived from user input. Additionally, changing this behavior may cause the test to behave differently on different platforms (Windows vs Unix-like systems).
While this is a revert, consider whether this change is intentional and necessary. If the script works correctly with shell=False, that would be the safer option.
| with tempfile.TemporaryDirectory() as folder: | ||
| filename = os.path.join(folder, "oci.tar") | ||
| filename2 = os.path.join(folder, "oci2.tar") | ||
| filename2 = os.path.join(self.path, "oci2.tar") |
There was a problem hiding this comment.
Writing the tar file to self.path (the test directory) instead of the temporary directory will create test artifacts in the repository directory. This can cause issues with version control, test isolation, and cleanup. The tar file oci2.tar will persist after the test completes.
Consider using the temporary directory for all test artifacts to ensure proper cleanup and test isolation.
2 participants
Reverts #9477