Conversation
…compose, docker compose programm
Multi-service compose files (like komodo) have service names different from app_code. The compose file defines 'core', 'periphery', 'ferretdb' services, not 'komodo'. Changes: - Remove app_code from pull/stop/rm/up commands - operate on all services - Remove --no-deps flag to allow proper multi-service startup - Add system container health check support - Handle directory-exists-as-file issue when writing configs
When deploy_app receives env_vars, write them to .env file in the compose directory. This supports compose files that use 'env_file: .env' directive. The env_vars are also still passed as process environment variables for compatibility with compose files that use variable substitution.
When docker-compose.yml references env_file paths that don't exist, create empty placeholder files to prevent 'file not found' errors. This handles the case where: 1. Compose file uses env_file: /path/to/.env 2. But no .env content was provided in the deploy_app command 3. Previously this would cause docker compose to fail Now an empty .env file is created with a comment, allowing the containers to start (though they may need proper env vars later).
- Add ConfigureProxyCommand struct with domain_names, forward_host, forward_port, ssl options - Add handler that authenticates with NPM API and creates/updates/deletes proxy hosts - Support for Let's Encrypt SSL, HTTP/2, and websocket upgrade - Actions: create, update, delete
- Create src/connectors/ module for external service integrations - Move NPM API logic to connectors/npm.rs with NpmClient struct - Simplify handle_configure_proxy to use NpmClient - Better separation of concerns and reusability
There was a problem hiding this comment.
Pull request overview
This PR wires up app deployment and configuration management end-to-end: it enriches the Vault client and stacker command set, adds an Nginx Proxy Manager connector, extends Docker-based deployment orchestration, and improves operational visibility (banner, metrics, server/containers introspection).
Changes:
- Expanded Vault client and stacker commands to support bulk config fetch/apply, config drift detection, combined config+deploy flows, and richer health/logs handling, plus new exec/server-resources/list-containers commands.
- Introduced a dedicated Nginx Proxy Manager connector and a
configure_proxystacker command, along with compose detection and improved docker-compose orchestration semantics in agent-side deployment. - Updated runtime environment integration: more informative startup banner, Docker CLI mounts for in-container compose usage, and new documentation (Vault/token strategy and app deployment plan) plus changelog and VS Code task updates.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| status.pid | Adds a tracked PID file for the running agent; likely an unintended runtime artifact that should not be versioned. |
| src/security/vault_client.rs | Documents and refines Vault KV response models and AppConfig, adds detailed security commentary, and introduces more flexible config path mapping and bulk config helpers, aligning with the per-deployment Vault layout. |
| src/main.rs | Extends the startup banner to show mode, dashboard/base URL, Vault URL, agent ID, auth/Stacker status, and build/runtime flags inferred from env and config.json. |
| src/lib.rs | Exposes the new connectors module at the crate root so other modules (e.g., stacker) can use external service clients. |
| src/connectors/npm.rs | Implements an Nginx Proxy Manager API client (config, auth, create/delete/list/find proxy hosts) plus basic tests for config defaults and request serialization. |
| src/connectors/mod.rs | Introduces the connectors module and re-exports NpmClient for convenient use across the crate. |
| src/commands/stacker.rs | Greatly expands the stacker command model and handlers: new remove/fetch-all/deploy-with-configs/config-diff/configure-proxy/exec/server-resources/list-containers commands, improved normalization/validation, docker-compose variant detection, env/config writing, exec-with-output, and comprehensive tests. |
| src/agent/docker.rs | Adds exec_in_container_with_output to run commands in containers and capture exit code plus separate stdout/stderr, used by the new exec stacker command. |
| docs/APP_DEPLOYMENT.md | Introduces a design document describing the Vault-based token/config layout, per-deployment token model, and higher-level app configuration deployment strategy. |
| docker-compose.yml | Mounts the host Docker CLI and compose plugin, plus /home/trydirect, into the agent and compose-agent containers so they can run docker/compose for deploy/remove operations. |
| docker-compose-dev.yml | Mirrors the production compose changes for the dev stack, mounting the host Docker CLI and compose plugin into the dev container. |
| CHANGELOG.md | Records the new Vault config, deployment, exec/server-resources/list-containers, and config management capabilities, including parameter summaries and implementation notes. |
| .vscode/tasks.json | Adds a VS Code task to run the status panel binary in daemon mode with config.json, useful for quickly checking the new startup banner and behavior. |
| - Returns structured JSON with system resource data | ||
|
|
||
| - `ListContainersCommand` / `stacker.list_containers`: List deployment containers | ||
| - Parameters: deployment_hash, include_health, include_logs, log_lines (1-1000) |
There was a problem hiding this comment.
The ListContainersCommand entry here documents log_lines as accepting a range of 1–1000, but the implementation in commands/stacker.rs clamps log_lines to 1–100 (self.log_lines = self.log_lines.clamp(1, 100)). To avoid confusion for API consumers, either update the documented range to 1–100 or relax the clamp in the code to match the stated 1–1000 range.
| - Parameters: deployment_hash, include_health, include_logs, log_lines (1-1000) | |
| - Parameters: deployment_hash, include_health, include_logs, log_lines (1-100) |
| @@ -0,0 +1 @@ | |||
| 34788 | |||
There was a problem hiding this comment.
Committing a live PID file to the repository will cause spurious diffs on every run and can break deployment/packaging flows that assume PID files are runtime artifacts; this file should be removed from source control and added to .gitignore instead.
| if let Some(hash) = &agent_cmd.deployment_hash { | ||
| self.deployment_hash = hash.clone(); | ||
| } | ||
| } |
There was a problem hiding this comment.
ExecCommand::with_command_context only propagates deployment_hash from the AgentCommand but does not fill app_code, whereas validate() requires a non-empty app_code and other commands (e.g. HealthCommand, DeployAppCommand, RemoveAppCommand) copy both fields. This means exec commands that rely on the top-level app_code field (and omit app_code in the JSON payload) will start failing validation. Consider mirroring the pattern used in the other commands and copy agent_cmd.app_code into self.app_code when it is empty.
| } | |
| } | |
| if self.app_code.is_empty() { | |
| if let Some(code) = &agent_cmd.app_code { | |
| self.app_code = code.clone(); | |
| } | |
| } |
| /// Delete a proxy host by matching domain names | ||
| pub async fn delete_proxy_host(&mut self, domain_names: &[String]) -> Result<ProxyHostResult> { | ||
| let token = self.ensure_authenticated().await?.to_string(); | ||
|
|
||
| // First, list all proxy hosts to find the matching one | ||
| let response = self | ||
| .client | ||
| .get(format!("{}/api/nginx/proxy-hosts", self.config.host)) | ||
| .header("Authorization", format!("Bearer {}", token)) | ||
| .send() | ||
| .await | ||
| .context("Failed to list proxy hosts from NPM")?; | ||
|
|
||
| let hosts: Vec<Value> = response.json().await.unwrap_or_default(); | ||
|
|
There was a problem hiding this comment.
In delete_proxy_host, the code parses the response body into hosts without checking response.status(), so HTTP errors from NPM (e.g., 401/403/500) will be silently treated as "no matching proxy host" and reported as a successful, idempotent delete. This makes it hard to detect authentication or connectivity issues; consider checking status.is_success() (similar to create_proxy_host) and returning an error when the list request fails instead of continuing with an empty host list.
| .await | ||
| .context("Failed to list proxy hosts from NPM")?; | ||
|
|
||
| let hosts: Vec<Value> = response.json().await.unwrap_or_default(); |
There was a problem hiding this comment.
Similarly to delete_proxy_host, list_proxy_hosts deserializes the response body into hosts without inspecting the HTTP status, so auth or server errors will be silently mapped to an empty list. For operational visibility, it would be safer to check response.status().is_success() and return an error on non-success status codes rather than hiding the failure.
| let hosts: Vec<Value> = response.json().await.unwrap_or_default(); | |
| if !response.status().is_success() { | |
| let status = response.status(); | |
| let body = response.text().await.unwrap_or_default(); | |
| return Err(anyhow::anyhow!( | |
| "Failed to list proxy hosts from NPM: HTTP {} - {}", | |
| status, | |
| body | |
| )); | |
| } | |
| let hosts: Vec<Value> = response | |
| .json() | |
| .await | |
| .context("Failed to parse proxy hosts response from NPM")?; |
| //! ├── {app_name_1}/ | ||
| //! │ └── config # App configuration (env vars, docker-compose, etc.) | ||
| //! └── {app_name_2}/ | ||
| //! └── config |
There was a problem hiding this comment.
The "Path Structure" documentation here still describes app configs as being stored at {prefix}/{deployment_hash}/apps/{app_name}/config, but config_path() below now builds {base_url}/v1/{prefix}/{deployment_hash}/apps/{app_code}/{config_type} (e.g., _compose, _env, _configs, _config). To avoid confusion when debugging Vault paths, consider updating this snippet to match the new path scheme and suffix conventions implemented in config_path.
| //! ├── {app_name_1}/ | |
| //! │ └── config # App configuration (env vars, docker-compose, etc.) | |
| //! └── {app_name_2}/ | |
| //! └── config | |
| //! ├── {app_code_1}/ | |
| //! │ ├── _compose # Docker Compose YAML for this app | |
| //! │ ├── _env # Flat environment variables for this app | |
| //! │ ├── _configs # Additional config files / blobs | |
| //! │ └── _config # Legacy / merged app config (backwards compatibility) | |
| //! └── {app_code_2}/ | |
| //! ├── _compose | |
| //! ├── _env | |
| //! ├── _configs | |
| //! └── _config |
| match data.action.as_str() { | ||
| "create" | "update" => { | ||
| let request = ProxyHostRequest { | ||
| domain_names: data.domain_names.clone(), | ||
| forward_host: forward_host.clone(), | ||
| forward_port: data.forward_port, | ||
| ssl_enabled: data.ssl_enabled, | ||
| ssl_forced: data.ssl_forced, | ||
| http2_support: data.http2_support, | ||
| }; | ||
|
|
||
| match npm_client.create_proxy_host(&request).await { | ||
| Ok(proxy_result) => { | ||
| if proxy_result.success { | ||
| result.result = Some(json!({ | ||
| "type": "configure_proxy", | ||
| "action": data.action, | ||
| "deployment_hash": data.deployment_hash, | ||
| "app_code": data.app_code, | ||
| "status": "success", | ||
| "proxy_host_id": proxy_result.proxy_host_id, | ||
| "domain_names": data.domain_names, | ||
| "forward_host": forward_host, | ||
| "forward_port": data.forward_port, | ||
| "ssl_enabled": data.ssl_enabled, | ||
| "created_at": now_timestamp(), | ||
| })); | ||
| } else { | ||
| let error = make_error("npm_create_failed", &proxy_result.message, None); | ||
| result.status = "error".to_string(); | ||
| result.error = Some(error.message.clone()); | ||
| } | ||
| } | ||
| Err(e) => { | ||
| let error = | ||
| make_error("npm_error", "NPM operation failed", Some(e.to_string())); | ||
| result.status = "error".to_string(); | ||
| result.error = Some(error.message.clone()); | ||
| } | ||
| } | ||
| } | ||
| "delete" => match npm_client.delete_proxy_host(&data.domain_names).await { | ||
| Ok(proxy_result) => { | ||
| if proxy_result.success { | ||
| result.result = Some(json!({ | ||
| "type": "configure_proxy", | ||
| "action": "delete", | ||
| "deployment_hash": data.deployment_hash, | ||
| "app_code": data.app_code, | ||
| "status": "success", | ||
| "deleted_proxy_host_id": proxy_result.proxy_host_id, | ||
| "message": proxy_result.message, | ||
| "deleted_at": now_timestamp(), | ||
| })); | ||
| } else { | ||
| let error = make_error("npm_delete_failed", &proxy_result.message, None); | ||
| result.status = "error".to_string(); | ||
| result.error = Some(error.message.clone()); | ||
| } | ||
| } | ||
| Err(e) => { | ||
| let error = make_error("npm_error", "NPM operation failed", Some(e.to_string())); | ||
| result.status = "error".to_string(); | ||
| result.error = Some(error.message.clone()); | ||
| } | ||
| }, | ||
| _ => { | ||
| let error = make_error( | ||
| "invalid_action", | ||
| format!( | ||
| "Unknown action: {}. Valid actions: create, update, delete", | ||
| data.action | ||
| ), | ||
| None, | ||
| ); | ||
| result.status = "error".to_string(); | ||
| result.error = Some(error.message.clone()); | ||
| } |
There was a problem hiding this comment.
handle_configure_proxy treats both "create" and "update" actions identically by always calling create_proxy_host, but ConfigureProxyCommand and the changelog document update as a distinct action. If the intent is to support true updates (e.g., modifying an existing proxy host), consider either implementing an explicit update path (look up the existing host and call the appropriate NPM endpoint) or narrowing the accepted actions to those actually supported to avoid misleading API consumers.
| if let Some(hash) = &agent_cmd.deployment_hash { | ||
| self.deployment_hash = hash.clone(); | ||
| } | ||
| } |
There was a problem hiding this comment.
ConfigureProxyCommand::with_command_context only copies deployment_hash from AgentCommand and ignores app_code, but validate() requires app_code to be non-empty. For callers that rely on the top-level app_code field and omit app_code in the JSON payload (consistent with other commands), this will cause validation to fail unexpectedly. To keep behavior consistent with commands like HealthCommand and DeployAppCommand, also copy agent_cmd.app_code into self.app_code when it is empty.
| } | |
| } | |
| if self.app_code.is_empty() { | |
| if let Some(code) = &agent_cmd.app_code { | |
| self.app_code = code.clone(); | |
| } | |
| } |
| - `ServerResourcesCommand` / `stacker.server_resources`: Collect server metrics | ||
| - Parameters: deployment_hash, include_disk, include_network, include_processes | ||
| - Uses MetricsCollector for CPU, memory, disk, network, and process info | ||
| - Returns structured JSON with system resource data | ||
|
|
||
| - `ListContainersCommand` / `stacker.list_containers`: List deployment containers | ||
| - Parameters: deployment_hash, include_health, include_logs, log_lines (1-1000) | ||
| - Returns container list with status, health info, and optional recent logs |
There was a problem hiding this comment.
The description for ServerResourcesCommand lists an include_processes parameter and claims network and process info are returned, but the actual struct in commands/stacker.rs only has include_disk and include_network, and handle_server_resources currently exposes only CPU/memory/disk (with a placeholder network note and no process data). Please update this section either to match the implemented fields/metrics or extend the implementation to provide the documented behavior.
1 participant
No description provided.