Skip to content

Add required extra Docker network attachments#88

Draft
adamlevineagent wants to merge 2 commits into
gotempsh:mainfrom
adamlevineagent:elaine/required-extra-docker-networks
Draft

Add required extra Docker network attachments#88
adamlevineagent wants to merge 2 commits into
gotempsh:mainfrom
adamlevineagent:elaine/required-extra-docker-networks

Conversation

@adamlevineagent

Copy link
Copy Markdown
Contributor

Summary

  • add TEMPS_DOCKER_EXTRA_NETWORKS for self-hosted installs that need app containers attached to external Docker dependency networks before startup
  • add DeployRequest.extra_networks for future per-request attachments while keeping default behavior empty/backwards compatible
  • fail deployment when a configured required network is missing or cannot be attached, avoiding green deploys with dependency DNS failures
  • document the operator env var and update deployer sample struct construction

Verification

  • cargo fmt
  • cargo check -p temps-config -p temps-deployer
  • cargo test -p temps-deployer test_normalize_extra_networks_drops_empty_duplicates_primary_and_overlay -- --nocapture
  • cargo test -p temps-deployer test_deploy_request_creation -- --nocapture

Note: cargo check -p temps-config -p temps-deployer -p temps-deployments still fails on Windows in existing docker_cleanup_service code using Docker::connect_with_unix_defaults and inferred cleanup result types; this patch did not touch that path.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds operator-configurable required Docker network attachments so deployed app containers can join external dependency networks (e.g., sibling Docker Compose networks) before starting, and fail the deploy if those networks are missing/unattachable (avoiding “green” deploys with broken DNS/dependencies).

Changes:

  • Introduces TEMPS_DOCKER_EXTRA_NETWORKS and plumbs it into the deployer runtime.
  • Adds DeployRequest.extra_networks (serde-defaulted) and attaches required networks during deploy.
  • Updates docs, samples, and tests/struct literals for the new config/request fields.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
docs/reference/environment-variables/page.mdx Documents TEMPS_DOCKER_EXTRA_NETWORKS and operator usage.
crates/temps-config/src/service.rs Adds ServerConfig.docker_extra_networks and CSV env parsing.
crates/temps-deployer/src/plugin.rs Reads config and configures DockerRuntime with extra networks.
crates/temps-deployer/src/lib.rs Adds DeployRequest.extra_networks (defaulted) and updates tests.
crates/temps-deployer/src/docker.rs Implements normalization + required network attachment during deploy; adds unit tests for normalization/defaults.
crates/temps-deployer/src/remote.rs Updates test request construction to include extra_networks.
crates/temps-deployer/README.md Updates deployment example struct literal to include new/required fields.
crates/temps-deployments/src/jobs/deploy_image.rs Initializes extra_networks on deploy requests.
crates/temps-deployments/src/handlers/nodes.rs Updates test ServerConfig construction with docker_extra_networks.
crates/temps-status-page/src/tests.rs Updates test ServerConfig construction with docker_extra_networks.
crates/temps-email/src/services/tracking_service_integration_tests.rs Updates test ServerConfig construction with docker_extra_networks.
crates/temps-email/src/services/email_service.rs Updates test ServerConfig construction with docker_extra_networks.
crates/temps-email/src/handlers/tracking_tests.rs Updates test ServerConfig construction with docker_extra_networks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +399 to +408
for network in networks {
let exists = existing_networks
.iter()
.any(|candidate| candidate.name.as_deref() == Some(network.as_str()));
if !exists {
return Err(DeployerError::NetworkError(format!(
"required network '{}' does not exist",
network
)));
}
Comment on lines 1520 to +1545
// Create container
let container = self
.docker
.create_container(
Some(
bollard::query_parameters::CreateContainerOptionsBuilder::new()
.name(&request.container_name)
.build(),
),
container_config,
)
.await
.map_err(|e| {
DeployerError::DeploymentFailed(format!("Failed to create container: {}", e))
})?;

// Multi-host overlay: best-effort additional attach to `temps-overlay`
// (or whatever the operator configured). Containers always boot with
// their primary network interface (`temps-app-network`); the overlay
// attachment is purely additive and silently no-ops when the overlay
// network isn't present yet on this node.
self.maybe_attach_overlay(&container.id).await?;

self.attach_required_networks(&container.id, &request.extra_networks)
.await?;

Comment on lines +375 to +392
/// Attach required dependency networks before container start so Docker's
/// embedded DNS can resolve service names during app boot.
async fn attach_required_networks(
&self,
container_id: &str,
request_networks: &[String],
) -> Result<(), DeployerError> {
let mut requested = self.extra_networks.clone();
requested.extend(request_networks.iter().cloned());
let networks = normalize_extra_networks(
requested,
&self.network_name,
self.overlay_network.as_deref(),
);
if networks.is_empty() {
return Ok(());
}

@dviejokfs

Copy link
Copy Markdown
Contributor

Hi @adamlevineagent!

Thanks for the contribution! I think it's a feature needed for existing VPS with services.

Could you review and apply the copilot fixes?

@adamlevineagent

adamlevineagent commented May 12, 2026

Copy link
Copy Markdown
Contributor Author

Hi David, thank you, and genuinely thank you for making Temps. It is very cool, and we are happy to contribute back for use cases like ours where an app needs to join existing VPS / Docker Compose dependency networks.

I reviewed Copilot's comments and pushed a follow-up commit: 4b20cb92.

What changed:

  • Required network lookup now accepts either Docker network name or network ID before calling connect_network.
  • If a deploy fails after create_container during overlay attach, required network attach, container start, or post-start port inspection, the just-created container is removed before returning the error. That avoids leaving stopped/orphaned containers behind on the new required-network failure path.
  • Added coverage for:
    • name-or-ID network matching,
    • successful deploy attachment using a required network ID,
    • missing required network failure cleaning up the created container.

Verification run locally:

  • cargo fmt
  • cargo check -p temps-deployer
  • cargo test -p temps-deployer docker_tests::test_required_network_identifier_matches_name_or_id -- --nocapture
  • cargo test -p temps-deployer docker_tests::test_deploy -- --nocapture
  • git diff --check

One note: Docker is not reachable from this Codex environment, so the Docker-backed tests exercised the repo's existing Docker-unavailable skip path here; the pure name-or-ID unit test did run directly.

@dviejokfs

Copy link
Copy Markdown
Contributor

@adamlevineagent Nice! What other use cases did you run into? Also, it would mean the world to me to have your testimonial on the webpage to help people discover this project in the future :)

@dviejokfs

Copy link
Copy Markdown
Contributor

@adamlevineagent can you add changelog for this feature?

@adamlevineagent

adamlevineagent commented May 14, 2026 via email

Copy link
Copy Markdown
Contributor Author

@adamlevineagent adamlevineagent marked this pull request as draft May 14, 2026 16:22
@adamlevineagent

Copy link
Copy Markdown
Contributor Author

Actually hold on this, we're going to submit an updated pull once we've worked out some remaining issue that arose.

@dviejokfs

Copy link
Copy Markdown
Contributor

sounds good :) Let me know about the testimonial when you can

@dviejokfs

dviejokfs commented May 20, 2026

Copy link
Copy Markdown
Contributor

Hi @adamlevineagent any news about this? would be cool to have this for 0.1.0. I'm trying to release it next week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants