-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Container Registry #194
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?
Conversation
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.
Pull Request Overview
This PR consolidates Docker operations in the CLI and adds container registry functionality, with significant refactoring of the platform management system. The changes include cleanup of logging mechanisms and consolidation of rich console usage throughout the CLI.
- Refactored Docker operations to use a consolidated
DockerImage
class and centralized docker command execution - Added container registry
pull
command with API key authentication - Replaced scattered rich console usage with centralized logging functions
- Restructured platform CLI by removing several utility modules and consolidating functionality
Reviewed Changes
Copilot reviewed 32 out of 34 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
tests/cli/test_github.py | New comprehensive test suite for GitHub repository parsing functionality |
tests/cli/test_docker.py | New test suite for Docker image parsing and normalization |
tests/cli/test_config.py | New tests for user configuration management |
pyproject.toml | Updated test linting rules to allow security issues in tests |
dreadnode/user_config.py | Replaced rich.print with centralized print_info function |
dreadnode/main.py | Replaced rich console calls with centralized logging functions |
dreadnode/logging_.py | Consolidated rich console usage and added utility print functions |
dreadnode/cli/shared.py | Updated type alias from LogLevelLiteral to LogLevel |
dreadnode/cli/profile/cli.py | Replaced rich console calls with centralized logging functions |
dreadnode/cli/platform/version.py | New version management module replacing old schemas |
dreadnode/cli/platform/utils/versions.py | Removed utility module, functionality moved elsewhere |
dreadnode/cli/platform/utils/printing.py | Removed utility module, functionality consolidated in logging_ |
dreadnode/cli/platform/upgrade.py | Removed upgrade module, functionality moved to cli.py |
dreadnode/cli/platform/tag.py | New tag management utilities |
dreadnode/cli/platform/stop.py | Removed stop module, functionality moved to cli.py |
dreadnode/cli/platform/status.py | Removed status module, functionality moved to cli.py |
dreadnode/cli/platform/start.py | Removed start module, functionality moved to cli.py |
dreadnode/cli/platform/schemas.py | Removed schemas module, replaced by version.py |
dreadnode/cli/platform/login.py | Removed login module, functionality moved to compose.py |
dreadnode/cli/platform/env_mgmt.py | Updated imports and type references |
dreadnode/cli/platform/download.py | Significant refactoring to use new version management system |
dreadnode/cli/platform/docker_.py | Removed Docker utilities module, functionality moved to main docker.py |
dreadnode/cli/platform/constants.py | Updated constants to use new type casting approach |
dreadnode/cli/platform/configure.py | Removed configure module, functionality moved to cli.py |
dreadnode/cli/platform/compose.py | New compose management module with Docker utilities |
dreadnode/cli/platform/cli.py | Consolidated platform commands with inlined functionality |
dreadnode/cli/main.py | Added new pull command and updated console usage |
dreadnode/cli/github.py | Replaced rich console calls with centralized logging functions |
dreadnode/cli/docker.py | New centralized Docker operations module |
dreadnode/api/client.py | Renamed registry credentials method and simplified platform releases |
docs/sdk/main.mdx | Updated documentation to reflect console usage changes |
docs/sdk/api.mdx | Updated API documentation for renamed registry method |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
payload = { | ||
"tag": tag, | ||
"services": services, | ||
"cli_version": cli_version, | ||
} | ||
try: | ||
response = self.request("POST", "/platform/get-releases", json_data=payload) | ||
|
||
except RuntimeError as e: | ||
if "403" in str(e): | ||
raise RuntimeError("You do not have access to platform releases.") from e | ||
|
||
if "404" in str(e): | ||
if "Image not found" in str(e): | ||
raise RuntimeError("Image not found") from e | ||
from dreadnode.version import VERSION | ||
|
||
raise RuntimeError( | ||
f"Failed to get platform releases: {e}. The feature is likely disabled on this server" | ||
) from e | ||
raise |
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.
I only removed this as I think it's a poor pattern to reflect in the API client code. Might be aggressive to remove clarity from the errors for users - open to moving this somewhere else in the CLI
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.
Yeah, I was considering how best to provide thoughtful errors in a "public" SDk that attempts to access "private" features?
if not env_overrides: | ||
print_warning("No configuration changes specified.") | ||
return | ||
|
||
print_info("Setting environment overrides ...") | ||
build_compose_override_file(PLATFORM_SERVICES, current_version) | ||
build_env_file(current_version.configure_overrides_env_file, **env_overrides) | ||
print_info( | ||
f"Configuration written to {current_version.local_path}.\n\n" | ||
"These will take effect the next time the platform is started. " | ||
"You can modify or remove them at any time." | ||
) |
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.
Generally did a lot of inlining of helper functions that were only used in a single place. I don't think we should be fighting to keep code "small" in arbitrary places at the trade-off having to jump around to understand functionality + more arg typing + more docstrings.
@@ -0,0 +1,212 @@ | |||
import typing as t |
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.
Essentially using this file for the bonding layer between our LocalVersion object and the general docker helpers.
|
||
SupportedArchitecture = t.Literal["amd64", "arm64"] | ||
SUPPORTED_ARCHITECTURES: list[SupportedArchitecture] = ["amd64", "arm64"] | ||
SUPPORTED_ARCHITECTURES = t.cast("list[SupportedArchitecture]", t.get_args(SupportedArchitecture)) |
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.
Little more verbose, but helps prevent deltas in the code as changes to the literal reflect in the list
tag = tag or "latest" | ||
tag = add_tag_arch_suffix(tag) |
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.
Tried to make tag selection a little more flexible. Users can pass a version with/without an arch.
@@ -0,0 +1,40 @@ | |||
import platform |
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.
These functions could be in compose or version, but the cyclic issues pushed me to segment it.
I do wonder longterm about making Tag a structured object with some parsing and helpers so we aren't always doing translations.
raise ValueError(f"Unknown service: {service}") | ||
|
||
|
||
class VersionConfig(BaseModel): |
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.
Re-used our patterns from UserConfig here as versions.json is a file saved on disk. Object is charged with serializing itself back/forth from disk.
return DEFAULT_DOCKER_REGISTRY_LOCAL_PORT | ||
|
||
|
||
def get_registry(config: ServerConfig) -> str: |
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.
Helper code from the original CLI, I'm trusting it generally, but it's probably fragile and needs some more consideration later for on-prem instances when we get there.
payload = { | ||
"tag": tag, | ||
"services": services, | ||
"cli_version": cli_version, | ||
} | ||
try: | ||
response = self.request("POST", "/platform/get-releases", json_data=payload) | ||
|
||
except RuntimeError as e: | ||
if "403" in str(e): | ||
raise RuntimeError("You do not have access to platform releases.") from e | ||
|
||
if "404" in str(e): | ||
if "Image not found" in str(e): | ||
raise RuntimeError("Image not found") from e | ||
from dreadnode.version import VERSION | ||
|
||
raise RuntimeError( | ||
f"Failed to get platform releases: {e}. The feature is likely disabled on this server" | ||
) from e | ||
raise |
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.
Yeah, I was considering how best to provide thoughtful errors in a "public" SDk that attempts to access "private" features?
Key Changes:
platform
CLIpull
command with api_key auth to the server-side registriesGenerated Summary:
get_container_registry_credentials
toget_platform_registry_credentials
for clarity.get_platform_releases
by removing thecli_version
parameter, now defaulting to the importedVERSION
.Docker
CLI module that includes functionality for managing Docker operations, including pulling images, tagging, and retrieving container details.rich
library outputs with custom logging functions for better control over console output.try-except
statements in theget_platform_releases
method, streamlining error handling and logic flow.This summary was generated with ❤️ by rigging