-
Notifications
You must be signed in to change notification settings - Fork 98
I have tried to make a purge command, please give me feedback. Thank you! #2566
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,153 @@ | ||||||||||||||||||||||||||||||
| from __future__ import annotations | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| import json | ||||||||||||||||||||||||||||||
| import shutil | ||||||||||||||||||||||||||||||
| import subprocess | ||||||||||||||||||||||||||||||
| import textwrap | ||||||||||||||||||||||||||||||
| from pathlib import Path | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| from charmcraft.application.commands.base import CharmcraftCommand | ||||||||||||||||||||||||||||||
| from craft_cli import emit | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
Comment on lines
+1
to
+11
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| class PurgeCommand(CharmcraftCommand): | ||||||||||||||||||||||||||||||
| name = "purge" | ||||||||||||||||||||||||||||||
| help_msg = "Remove cached data and build containers" | ||||||||||||||||||||||||||||||
| overview = textwrap.dedent( | ||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||
| Remove cached data and build containers created by Charmcraft. | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| By default, this command removes the pip cache and stopped build | ||||||||||||||||||||||||||||||
| containers. Running containers are preserved unless explicitly | ||||||||||||||||||||||||||||||
| requested. | ||||||||||||||||||||||||||||||
|
Comment on lines
+20
to
+22
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| Use this command to recover disk space or clean up broken build | ||||||||||||||||||||||||||||||
| environments. | ||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def fill_parser(self, parser) -> None: | ||||||||||||||||||||||||||||||
| super().fill_parser(parser) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| parser.add_argument( | ||||||||||||||||||||||||||||||
| "--only-cache", | ||||||||||||||||||||||||||||||
| action="store_true", | ||||||||||||||||||||||||||||||
| help="Clear only the pip cache", | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
| parser.add_argument( | ||||||||||||||||||||||||||||||
| "--only-builders", | ||||||||||||||||||||||||||||||
| action="store_true", | ||||||||||||||||||||||||||||||
| help="Remove only stopped build containers", | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
| parser.add_argument( | ||||||||||||||||||||||||||||||
| "--include-running", | ||||||||||||||||||||||||||||||
| action="store_true", | ||||||||||||||||||||||||||||||
| help="Include running build containers", | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
| parser.add_argument( | ||||||||||||||||||||||||||||||
| "--all", | ||||||||||||||||||||||||||||||
| action="store_true", | ||||||||||||||||||||||||||||||
| help="Remove all caches and build containers, including bases", | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| # Public entry point | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def run(self, parsed_args) -> None: | ||||||||||||||||||||||||||||||
|
Comment on lines
+53
to
+57
|
||||||||||||||||||||||||||||||
| # Public entry point | |
| def run(self, parsed_args) -> None: | |
| def run(self, parsed_args) -> None: | |
| """Execute the purge command based on the provided arguments.""" |
Copilot
AI
Feb 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cache directory is hardcoded to Path.home() / ".cache" / "charmcraft", which doesn't respect XDG environment variables or platform conventions. The codebase uses platformdirs.user_cache_path(appname="charmcraft") in charmcraft/env.py:37 for determining the cache path, which properly handles XDG_CACHE_HOME and platform-specific conventions. This method should be used instead to ensure consistency and proper cross-platform support.
Copilot
AI
Feb 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling for shutil.rmtree. If the removal fails due to permission errors or file system issues, the exception will propagate uncaught. Consider wrapping this in a try-except block and providing a user-friendly error message using emit.warning or similar, as done in _list_lxd_containers at lines 126-131.
| shutil.rmtree(cache_dir) | |
| try: | |
| shutil.rmtree(cache_dir) | |
| except OSError as exc: | |
| emit.warning(f"Failed to remove cache directory {cache_dir}: {exc}") |
Copilot
AI
Feb 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar placeholder comment style as above. Consider removing or replacing with proper docstrings.
Copilot
AI
Feb 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling for subprocess.run when deleting containers. If the deletion fails (e.g., due to LXD daemon issues, permission problems, or container state issues), the CalledProcessError will propagate uncaught and crash the command. This could leave the command in an inconsistent state where some containers are deleted but others are not. Consider wrapping this in a try-except block to handle failures gracefully and continue attempting to delete other containers, similar to the error handling pattern used in _list_lxd_containers.
| subprocess.run( | |
| ["lxc", "delete", "--force", name], | |
| check=True, | |
| ) | |
| try: | |
| subprocess.run( | |
| ["lxc", "delete", "--force", name], | |
| check=True, | |
| ) | |
| except FileNotFoundError: | |
| emit.warning("LXD is not installed; stopping container cleanup.") | |
| break | |
| except subprocess.CalledProcessError as exc: | |
| emit.warning(f"Failed to delete LXD container {name!r}: {exc}") |
Copilot
AI
Feb 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dict type annotation should specify the expected structure. Based on the usage in _filter_builder_containers, containers have at least "name" and "status" keys. Consider using a more specific type like list[dict[str, Any]] or defining a TypedDict for the container structure to improve type safety and documentation.
Copilot
AI
Feb 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling for json.loads. If the LXD output is malformed or not valid JSON, this will raise a JSONDecodeError and crash the command. Consider wrapping this in a try-except block and returning an empty list with a warning message if JSON parsing fails.
| return json.loads(result.stdout) | |
| try: | |
| return json.loads(result.stdout) | |
| except json.JSONDecodeError as exc: | |
| emit.warning(f"Failed to parse LXD JSON output: {exc}; skipping container cleanup.") | |
| return [] |
Copilot
AI
Feb 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing test coverage for the new PurgeCommand. Other commands in the codebase have corresponding test files (e.g., tests/unit/commands/test_lifecycle.py for lifecycle commands). The command has several behaviors that should be tested: cache removal, container filtering, error handling for missing LXD, different flag combinations (--only-cache, --only-builders, --include-running, --all), and edge cases like permission errors or malformed JSON from LXD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace on this line. Most linters flag this as a style issue.