Fix shared-cache option naming consistency - #2651
Fix shared-cache option naming consistency#2651Sylvester Kaczmarek (sylvesterkaczmarek) wants to merge 4 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🔵 Needs a closer look
The shared-cache delete path can unintentionally delete the entire container when --delete is used without --all and --model_hash is omitted, because delete_blob(None) can enumerate all blobs.
Pull request overview
This PR improves CLI consistency in Olive by making the olive shared-cache command use the same shared-cache option names (--account_name, --container_name) that are used elsewhere in the CLI, while preserving the legacy flags (--account, --container) as aliases for backward compatibility.
Changes:
- Make
--account_name/--container_namethe canonical flags forolive shared-cache, retaining--account/--containeras aliases. - Update the shared-cache command implementation to read the canonical parsed argument names.
- Add a unit test to validate that both canonical and legacy option names parse into the same resulting namespace fields.
File summaries
| File | Description |
|---|---|
olive/cli/shared_cache.py |
Adds canonical shared-cache option names and switches runtime usage to args.account_name / args.container_name. |
test/cli/test_shared_cache.py |
Adds parser coverage ensuring both canonical and legacy flags populate the canonical argument destinations. |
Review details
Suppressed comments (1)
olive/cli/shared_cache.py:62
- When
--deleteis used without--all,delete_blob(self.args.model_hash)is called even if--model_hashwas not provided. InAzureContainerClientFactory.delete_blob, aNoneblob_name results inlist_blobs(None)which can enumerate all blobs, soolive shared-cache --delete ...could unintentionally delete the entire container.
container_client_factory = AzureContainerClientFactory(self.args.account_name, self.args.container_name)
if self.args.delete:
if self.args.all:
if self.args.yes:
container_client_factory.delete_all()
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Addressed the delete-safety finding. |
Describe your changes
The standalone
olive shared-cachecommand uses--accountand--container, while Olive's shared-cache options elsewhere use--account_nameand--container_name. Equivalent shared-cache configuration therefore uses different CLI names depending on the command.This change makes
--account_nameand--container_namethe canonical option names forshared-cachewhile retaining--accountand--containeras backward-compatible aliases, so existing scripts continue to work.This addresses the shared-cache CLI consistency finding in #2516.
Tests
Adds parser coverage for both the consistent option names and the legacy aliases.
Checklist before requesting a review