Fix/network container improvements

FULL_HELP_DOCS.md

@@ -1062,7 +1062,7 @@ Start a container running a Stellar node, RPC, API, and friendbot (faucet).
 
 By default, when starting a testnet container, without any optional arguments, it will run the equivalent of the following docker command:
 
-`docker run --rm -p 8000:8000 --name stellar stellar/quickstart:testing --testnet --enable-soroban-rpc`
+`docker run --rm -p 8000:8000 --name stellar stellar/quickstart:testing --testnet --enable rpc,horizon`
 
 **Usage:** `stellar network start [OPTIONS] <NETWORK>`
 
@@ -1076,6 +1076,7 @@ By default, when starting a testnet container, without any optional arguments, i
 ###### **Options:**
 
 * `-d`, `--docker-host <DOCKER_HOST>` — Optional argument to override the default docker host. This is useful when you are using a non-standard docker host path for your Docker-compatible container runtime, e.g. Docker Desktop defaults to $HOME/.docker/run/docker.sock instead of /var/run/docker.sock
+* `--name <NAME>` — Optional argument to specify the container name
 * `-l`, `--limits <LIMITS>` — Optional argument to specify the limits for the local network only
 * `-p`, `--ports-mapping <PORTS_MAPPING>` — Argument to specify the `HOST_PORT:CONTAINER_PORT` mapping
 
@@ -1091,14 +1092,11 @@ By default, when starting a testnet container, without any optional arguments, i
 
 Stop a network started with `network start`. For example, if you ran `stellar network start local`, you can use `stellar network stop local` to stop it.
 
-**Usage:** `stellar network stop [OPTIONS] <NETWORK>`
+**Usage:** `stellar network stop [OPTIONS] <NAME>`
 
 ###### **Arguments:**
 
-* `<NETWORK>` — Network to stop
-
-  Possible values: `local`, `testnet`, `futurenet`, `pubnet`
-
+* `<NAME>` — Container to stop
 
 ###### **Options:**
 
@@ -1114,24 +1112,21 @@ Commands to start, stop and get logs for a quickstart container
 
 ###### **Subcommands:**
 
-* `logs` — Tail logs of a running network container
-* `start` — Start network
-* `stop` — Stop a network started with `network container start`. For example, if you ran `network container start local`, you can use `network container stop local` to stop it
+* `logs` — Get logs from a running network container
+* `start` — Start a container running a Stellar node, RPC, API, and friendbot (faucet)
+* `stop` — Stop a network container started with `network container start`
 
 
 
 ## `stellar network container logs`
 
-Tail logs of a running network container
+Get logs from a running network container
 
-**Usage:** `stellar network container logs [OPTIONS] <NETWORK>`
+**Usage:** `stellar network container logs [OPTIONS] <NAME>`
 
 ###### **Arguments:**
 
-* `<NETWORK>` — Network to tail
-
-  Possible values: `local`, `testnet`, `futurenet`, `pubnet`
-
+* `<NAME>` — Container to get logs from
 
 ###### **Options:**
 
@@ -1141,15 +1136,13 @@ Tail logs of a running network container
 
 ## `stellar network container start`
 
-Start network
-
 Start a container running a Stellar node, RPC, API, and friendbot (faucet).
 
-`stellar network start NETWORK [OPTIONS]`
+`stellar network container start NETWORK [OPTIONS]`
 
 By default, when starting a testnet container, without any optional arguments, it will run the equivalent of the following docker command:
 
-`docker run --rm -p 8000:8000 --name stellar stellar/quickstart:testing --testnet --enable-soroban-rpc`
+`docker run --rm -p 8000:8000 --name stellar stellar/quickstart:testing --testnet --enable rpc,horizon`
 
 **Usage:** `stellar network container start [OPTIONS] <NETWORK>`
 
@@ -1163,6 +1156,7 @@ By default, when starting a testnet container, without any optional arguments, i
 ###### **Options:**
 
 * `-d`, `--docker-host <DOCKER_HOST>` — Optional argument to override the default docker host. This is useful when you are using a non-standard docker host path for your Docker-compatible container runtime, e.g. Docker Desktop defaults to $HOME/.docker/run/docker.sock instead of /var/run/docker.sock
+* `--name <NAME>` — Optional argument to specify the container name
 * `-l`, `--limits <LIMITS>` — Optional argument to specify the limits for the local network only
 * `-p`, `--ports-mapping <PORTS_MAPPING>` — Argument to specify the `HOST_PORT:CONTAINER_PORT` mapping
 
@@ -1174,16 +1168,13 @@ By default, when starting a testnet container, without any optional arguments, i
 
 ## `stellar network container stop`
 
-Stop a network started with `network container start`. For example, if you ran `network container start local`, you can use `network container stop local` to stop it
+Stop a network container started with `network container start`
 
-**Usage:** `stellar network container stop [OPTIONS] <NETWORK>`
+**Usage:** `stellar network container stop [OPTIONS] <NAME>`
 
 ###### **Arguments:**
 
-* `<NETWORK>` — Network to stop
-
-  Possible values: `local`, `testnet`, `futurenet`, `pubnet`
-
+* `<NAME>` — Container to stop
 
 ###### **Options:**
 

cmd/soroban-cli/src/commands/network/container.rs

@@ -10,19 +10,17 @@ pub type StopCmd = stop::Cmd;
 
 #[derive(Debug, clap::Subcommand)]
 pub enum Cmd {
-    /// Tail logs of a running network container
+    /// Get logs from a running network container
     Logs(logs::Cmd),
-    /// Start network
-    ///
     /// Start a container running a Stellar node, RPC, API, and friendbot (faucet).
     ///
-    /// `stellar network start NETWORK [OPTIONS]`
+    /// `stellar network container start NETWORK [OPTIONS]`
     ///
     /// By default, when starting a testnet container, without any optional arguments, it will run the equivalent of the following docker command:
     ///
-    /// `docker run --rm -p 8000:8000 --name stellar stellar/quickstart:testing --testnet --enable-soroban-rpc`
+    /// `docker run --rm -p 8000:8000 --name stellar stellar/quickstart:testing --testnet --enable rpc,horizon`
     Start(start::Cmd),
-    /// Stop a network started with `network container start`. For example, if you ran `network container start local`, you can use `network container stop local` to stop it.
+    /// Stop a network container started with `network container start`.
     Stop(stop::Cmd),
 }
 

cmd/soroban-cli/src/commands/network/container/logs.rs

@@ -1,8 +1,8 @@
 use futures_util::TryStreamExt;
 
-use crate::commands::network::container::shared::{
-    connect_to_docker, Error as ConnectionError, Network, DOCKER_HOST_HELP,
-};
+use crate::commands::network::container::shared::Error as ConnectionError;
+
+use super::shared::{Args, Name};
 
 #[derive(thiserror::Error, Debug)]
 pub enum Error {
@@ -15,17 +15,17 @@ pub enum Error {
 
 #[derive(Debug, clap::Parser, Clone)]
 pub struct Cmd {
-    /// Network to tail
-    pub network: Network,
+    #[command(flatten)]
+    pub container_args: Args,
 
-    #[arg(short = 'd', long, help = DOCKER_HOST_HELP, env = "DOCKER_HOST")]
-    pub docker_host: Option<String>,
+    /// Container to get logs from
+    pub name: String,
 }
 
 impl Cmd {
     pub async fn run(&self) -> Result<(), Error> {
-        let container_name = format!("stellar-{}", self.network);
-        let docker = connect_to_docker(&self.docker_host).await?;
+        let container_name = Name::new(Some(self.name.clone()), None).get_internal_container_name();
+        let docker = self.container_args.connect_to_docker().await?;
         let logs_stream = &mut docker.logs(
             &container_name,
             Some(bollard::container::LogsOptions {

cmd/soroban-cli/src/commands/network/container/shared.rs

@@ -31,7 +31,81 @@ pub enum Error {
     UnsupportedURISchemeError { uri: String },
 }
 
-#[derive(ValueEnum, Debug, Clone, PartialEq)]
+#[derive(Debug, clap::Parser, Clone)]
+pub struct Args {
+    /// Optional argument to override the default docker host. This is useful when you are using a non-standard docker host path for your Docker-compatible container runtime, e.g. Docker Desktop defaults to $HOME/.docker/run/docker.sock instead of /var/run/docker.sock
+    #[arg(short = 'd', long, help = DOCKER_HOST_HELP, env = "DOCKER_HOST")]
+    pub docker_host: Option<String>,
+}
+
+impl Args {
+    pub(crate) fn get_additional_flags(&self) -> String {
+        self.docker_host
+            .as_ref()
+            .map(|docker_host| format!("--docker-host {docker_host}"))
+            .unwrap_or_default()
+    }
+
+    pub(crate) async fn connect_to_docker(&self) -> Result<Docker, Error> {
+        // if no docker_host is provided, use the default docker host:
+        // "unix:///var/run/docker.sock" on unix machines
+        // "npipe:////./pipe/docker_engine" on windows machines
+        let host = self.docker_host.as_ref().map_or_else(
+            || DEFAULT_DOCKER_HOST.to_string(),
+            std::string::ToString::to_string,
+        );
+
+        // this is based on the `connect_with_defaults` method which has not yet been released in the bollard crate
+        // https://github.com/fussybeaver/bollard/blob/0972b1aac0ad5c08798e100319ddd0d2ee010365/src/docker.rs#L660
+        let connection = match host.clone() {
+            // if tcp or http, use connect_with_http_defaults
+            // if unix and host starts with "unix://" use connect_with_unix
+            // if windows and host starts with "npipe://", use connect_with_named_pipe
+            // else default to connect_with_unix
+            h if h.starts_with("tcp://") || h.starts_with("http://") => {
+                Docker::connect_with_http_defaults()
+            }
+            #[cfg(unix)]
+            h if h.starts_with("unix://") => {
+                Docker::connect_with_unix(&h, DEFAULT_TIMEOUT, API_DEFAULT_VERSION)
+            }
+            #[cfg(windows)]
+            h if h.starts_with("npipe://") => {
+                Docker::connect_with_named_pipe(&h, DEFAULT_TIMEOUT, API_DEFAULT_VERSION)
+            }
+            _ => {
+                return Err(Error::UnsupportedURISchemeError {
+                    uri: host.to_string(),
+                });
+            }
+        }?;
+
+        match check_docker_connection(&connection).await {
+            Ok(()) => Ok(connection),
+            // If we aren't able to connect with the defaults, or with the provided docker_host
+            // try to connect with the default docker desktop socket since that is a common use case for devs
+            #[allow(unused_variables)]
+            Err(e) => {
+                // if on unix, try to connect to the default docker desktop socket
+                #[cfg(unix)]
+                {
+                    let docker_desktop_connection = try_docker_desktop_socket(&host)?;
+                    match check_docker_connection(&docker_desktop_connection).await {
+                        Ok(()) => Ok(docker_desktop_connection),
+                        Err(err) => Err(err)?,
+                    }
+                }
+
+                #[cfg(windows)]
+                {
+                    Err(e)?
+                }
+            }
+        }
+    }
+}
+
+#[derive(ValueEnum, Debug, Copy, Clone, PartialEq)]
 pub enum Network {
     Local,
     Testnet,
@@ -52,60 +126,25 @@ impl fmt::Display for Network {
     }
 }
 
-pub async fn connect_to_docker(docker_host: &Option<String>) -> Result<Docker, Error> {
-    // if no docker_host is provided, use the default docker host:
-    // "unix:///var/run/docker.sock" on unix machines
-    // "npipe:////./pipe/docker_engine" on windows machines
-
-    let host = docker_host
-        .clone()
-        .unwrap_or(DEFAULT_DOCKER_HOST.to_string());
-
-    // this is based on the `connect_with_defaults` method which has not yet been released in the bollard crate
-    // https://github.com/fussybeaver/bollard/blob/0972b1aac0ad5c08798e100319ddd0d2ee010365/src/docker.rs#L660
-    let connection = match host.clone() {
-        // if tcp or http, use connect_with_http_defaults
-        // if unix and host starts with "unix://" use connect_with_unix
-        // if windows and host starts with "npipe://", use connect_with_named_pipe
-        // else default to connect_with_unix
-        h if h.starts_with("tcp://") || h.starts_with("http://") => {
-            Docker::connect_with_http_defaults()
-        }
-        #[cfg(unix)]
-        h if h.starts_with("unix://") => {
-            Docker::connect_with_unix(&h, DEFAULT_TIMEOUT, API_DEFAULT_VERSION)
-        }
-        #[cfg(windows)]
-        h if h.starts_with("npipe://") => {
-            Docker::connect_with_named_pipe(&h, DEFAULT_TIMEOUT, API_DEFAULT_VERSION)
-        }
-        _ => {
-            return Err(Error::UnsupportedURISchemeError {
-                uri: host.to_string(),
-            });
+pub struct Name(Option<String>, Option<Network>);
+impl Name {
+    pub fn new(name: Option<String>, network: Option<Network>) -> Self {
+        Self(name, network)
+    }
+
+    pub fn get_internal_container_name(&self) -> String {
+        match (&self.0, &self.1) {
+            (Some(name), _) => format!("stellar-{}", name),
+            (None, Some(network)) => format!("stellar-{}", network),
+            (None, None) => panic!("Container name and/or network are required."),
         }
-    }?;
-
-    match check_docker_connection(&connection).await {
-        Ok(()) => Ok(connection),
-        // If we aren't able to connect with the defaults, or with the provided docker_host
-        // try to connect with the default docker desktop socket since that is a common use case for devs
-        #[allow(unused_variables)]
-        Err(e) => {
-            // if on unix, try to connect to the default docker desktop socket
-            #[cfg(unix)]
-            {
-                let docker_desktop_connection = try_docker_desktop_socket(&host)?;
-                match check_docker_connection(&docker_desktop_connection).await {
-                    Ok(()) => Ok(docker_desktop_connection),
-                    Err(err) => Err(err)?,
-                }
-            }
+    }
 
-            #[cfg(windows)]
-            {
-                Err(e)?
-            }
+    pub fn get_external_container_name(&self) -> String {
+        match (&self.0, &self.1) {
+            (Some(name), _) => name.to_string(),
+            (None, Some(network)) => network.to_string(),
+            (None, None) => panic!("Container name and/or network are required."),
         }
     }
 }
@@ -148,3 +187,61 @@ async fn check_docker_connection(docker: &Docker) -> Result<(), bollard::errors:
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    const NETWORK: Network = Network::Local;
+    const CONTAINER_NAME: &str = "test-name";
+
+    #[test]
+    fn test_get_internal_container_name_with_both_args() {
+        let name = Name::new(Some(CONTAINER_NAME.to_string()), Some(NETWORK));
+        assert_eq!(name.get_internal_container_name(), "stellar-test-name");
+    }
+
+    #[test]
+    fn test_get_internal_container_name_with_network() {
+        let name = Name::new(None, Some(NETWORK));
+        assert_eq!(name.get_internal_container_name(), "stellar-local");
+    }
+
+    #[test]
+    fn test_get_internal_container_name_with_name() {
+        // in practice, this would fail clap validation and we would never get here, but testing anyway
+        let name = Name::new(Some(CONTAINER_NAME.to_string()), None);
+        assert_eq!(name.get_internal_container_name(), "stellar-test-name");
+    }
+
+    #[test]
+    #[should_panic]
+    fn test_get_internal_container_name_with_neither_args() {
+        Name::new(None, None).get_internal_container_name();
+    }
+
+    #[test]
+    fn test_get_external_container_name_with_both_args() {
+        let name = Name::new(Some(CONTAINER_NAME.to_string()), Some(NETWORK));
+        assert_eq!(name.get_external_container_name(), "test-name");
+    }
+
+    #[test]
+    fn test_get_external_container_name_with_network() {
+        let name = Name::new(None, Some(NETWORK));
+        assert_eq!(name.get_external_container_name(), "local");
+    }
+
+    #[test]
+    fn test_get_external_container_name_with_name() {
+        // in practice, this would fail clap validation and we would never get here, but testing anyway
+        let name = Name::new(Some(CONTAINER_NAME.to_string()), None);
+        assert_eq!(name.get_external_container_name(), "test-name");
+    }
+
+    #[test]
+    #[should_panic]
+    fn test_get_external_container_name_with_neither_args() {
+        Name::new(None, None).get_external_container_name();
+    }
+}