feat(acrcache): implement dual flag support for ACR cache tag filtering with backward compatibility #69
Conversation
…ckward compatibility - Add new explicit parameters: --tag-equals, --tag-starts-with, --tag-ends-with, --tag-contains - Maintain backward compatibility with legacy parameters: --tag, --starts-with, --ends-with, --contains - New parameters take precedence when both legacy and new are provided - Add deprecation warnings for legacy parameters - Add comprehensive test coverage for dual flag functionality - Remove preview flag from 'acr cache' command group - Rename sync parameter from 'image' to 'tag' for consistency - Rewrite and clarify acrcache extension README with updated parameters and usage
| # Only set sync_referrers_str when the parameter is explicitly provided; otherwise, leave unchanged | ||
| sync_referrers_str = None | ||
| if sync_referrers is not None: | ||
| sync_referrers_str = "Enabled" if sync_referrers.lower() == 'enabled' else "Disabled" |
There was a problem hiding this comment.
Can you double check this logic in an unpdate command? If you don't set the --sync parameter explicitely, it will set it to disable. Is this what we want?
There was a problem hiding this comment.
Pull request overview
This pull request implements dual flag support for ACR cache tag filtering parameters with backward compatibility. The changes allow users to use new, more explicit parameter names (--tag-equals, --tag-starts-with, etc.) while maintaining support for legacy parameters (--tag, --starts-with, etc.) that now trigger deprecation warnings. The PR also removes the preview flag from the 'acr cache' command group and renames the sync parameter from 'image' to 'tag' for consistency.
Changes:
- Add new explicit tag filtering parameters with deprecation warnings for legacy parameters
- Remove preview flag from 'acr cache' command group, promoting it to GA
- Rename 'acr cache sync' parameter from 'image' to 'tag' for clarity and consistency
- Rewrite README documentation with updated parameters, examples, and migration guide
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/acrcache/azext_acrcache/cache.py | Added process_tag_filters function for parameter merging, implemented deprecation warnings, removed _separate_params function, updated sync function signature |
| src/acrcache/azext_acrcache/_params.py | Added new explicit tag filtering parameters (--tag-equals, etc.) and defined 'tag' parameter in separate context for sync command |
| src/acrcache/azext_acrcache/commands.py | Removed is_preview=True flag from command group registration |
| src/acrcache/azext_acrcache/tests/latest/test_cache.py | Added tests for dual flag support and parameter processing, removed mutual exclusivity validation tests, updated test for sync parameter rename |
| src/acrcache/README.rst | Complete rewrite with restructured documentation, updated parameter descriptions, added migration guide, and new usage examples |
Comments suppressed due to low confidence (1)
src/acrcache/azext_acrcache/cache.py:217
- The validation logic checks if
sync_referrers.lower() == 'enabled'on line 215, but this validation occurs after the code already attempts to call.lower()on line 212 whensync_referrers is not None. Ifsync_referrersis provided but is not a string (e.g., already an enum value), this could cause an AttributeError. Consider adding type validation or handling for non-string values before calling.lower().
sync_referrers_str = None
if sync_referrers is not None:
sync_referrers_str = "Enabled" if sync_referrers.lower() == 'enabled' else "Disabled"
# Validate sync_referrers requires activesync - check both when sync is provided and when it's not
if sync_referrers and sync_referrers.lower() == 'enabled':
if not sync or sync.lower() != 'activesync':
raise CLIError("Syncing referrers requires sync to be set to 'activesync'. Please update your cache rule configuration.")
| Requirements | ||
| ------------ | ||
|
|
||
| - Azure CLI version **2.55.0** or higher |
There was a problem hiding this comment.
The README states the minimum Azure CLI version is "2.55.0", but there's a discrepancy with previous documentation that mentioned "2.57.0" as the minimum version. Since this PR removes the preview flag from the command group (line 18 of commands.py), it's important to verify and consistently document the correct minimum CLI version across all documentation.
| - Azure CLI version **2.55.0** or higher | |
| - Azure CLI version **2.57.0** or higher |
| ) | ||
|
|
||
| with self.command_group('acr cache', acr_cache_util, is_preview=True) as g: | ||
| with self.command_group('acr cache', acr_cache_util) as g: |
There was a problem hiding this comment.
Can you comment what cases you tested here? The ones I see:
- Customer uses normal CLI and uses cache rules (PTC): shouldn't see preview
- Customer installs artifact sync cli extension and creates a cache rule with Artifact Sync: Sees preview
mabelegba
changed the title
3 participants
This checklist is used to make sure that common guidelines for a pull request are followed.
Changes Introduced:
--tag-equals,--tag-starts-with,--tag-ends-with,--tag-contains--tag,--starts-with,--ends-with,--contains--tag,--starts-with,--ends-with,--containsacr cachecommand group--imageto--tagfor consistencyRelated command
az acr cache listaz acr cache showaz acr cache createaz acr cache updateaz acr syncGeneral Guidelines
azdev style <YOUR_EXT>locally? (pip install azdevrequired)python scripts/ci/test_index.py -qlocally? (pip install wheel==0.30.0required)For new extensions:
About Extension Publish
There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update
src/index.jsonautomatically.You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify
src/index.json.