[Spacetime] Enhance errors #2779
Conversation
💛 Build succeeded, but was flaky
Failed CI StepsHistory |
| // Check if Docker daemon is running | ||
| if err := CheckDockerRunning(); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Instead of having to check it on every operation, I think we could create a new newComposeProvider() (*composeProvider, error) constructor that could run the check, when initializing it in the factory, here:
There was a problem hiding this comment.
I think the other providers (Environment and Serverless) could take advantage of this check too. IIRC these two providers run also docker (docker-compose) commands.
Just thinking that doing this check in the providers makes that elastic-package test or install commands do not run this check.
Maybe it's not needed, since all these commands will require to be the Elastic stack up... WDYT?
| logger.Error(": Docker daemon is not running or not accessible") | ||
| logger.Error(": Please make sure Docker is installed and running before executing commands that require Docker") |
There was a problem hiding this comment.
This is an internal method. I would prefer to log closer to the user.
| logger.Error(": Docker daemon is not running or not accessible") | |
| logger.Error(": Please make sure Docker is installed and running before executing commands that require Docker") |
| daemonError := fmt.Errorf("docker daemon is not running (stderr=%q): %w", errOutput.String(), err) | ||
| logger.Error(": Docker daemon is not running or not accessible") | ||
| logger.Error(": Please make sure Docker is installed and running before executing commands that require Docker") | ||
| return daemonError |
There was a problem hiding this comment.
Nit. You can directly return the error.
| daemonError := fmt.Errorf("docker daemon is not running (stderr=%q): %w", errOutput.String(), err) | |
| logger.Error(": Docker daemon is not running or not accessible") | |
| logger.Error(": Please make sure Docker is installed and running before executing commands that require Docker") | |
| return daemonError | |
| logger.Error(": Docker daemon is not running or not accessible") | |
| logger.Error(": Please make sure Docker is installed and running before executing commands that require Docker") | |
| return fmt.Errorf("docker daemon is not running (stderr=%q): %w", errOutput.String(), err) |
| errOutput := new(bytes.Buffer) | ||
| cmd.Stderr = errOutput | ||
|
|
||
| logger.Debugf("running command to check Docker daemon: %s", cmd) |
There was a problem hiding this comment.
I'd use Trace level for this message
| logger.Debugf("running command to check Docker daemon: %s", cmd) | |
| logger.Tracef("running command to check Docker daemon: %s", cmd) |
| // Check if Docker daemon is running | ||
| if err := CheckDockerRunning(); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
I think the other providers (Environment and Serverless) could take advantage of this check too. IIRC these two providers run also docker (docker-compose) commands.
Just thinking that doing this check in the providers makes that elastic-package test or install commands do not run this check.
Maybe it's not needed, since all these commands will require to be the Elastic stack up... WDYT?
| // CheckDaemonRunning checks if the Docker daemon is running. | ||
| // Returns nil if the daemon is running, or an error if it's not. | ||
| func CheckDaemonRunning() error { | ||
| cmd := exec.Command("docker", "info") |
There was a problem hiding this comment.
This commands has a large output. It should not be an issue, but maybe it could be format the output to just shown a specific value (e.g. ServerVersion).
| cmd := exec.Command("docker", "info") | |
| cmd := exec.Command("docker", "info", "--format", "{{ .ServerVersion }}") |
Summary
Closes https://github.com/elastic/ingest-dev/issues/5837
This is an exploratory PR aimed to improve errors displayed to the user during common commands.
The check added checks the status of Docker and ensures the daemon is running before trying to continue, which resolves issues where the CLI could 'hang' with no error output unless the user explicitly passed in the -d and or -v flags. Now we will perform a precheck and log to the user the issue.
I also explored adding a precheck for packages to be built before running install, but realized that the install command checks that already and then performs the build automatically, so the check is not needed.
Testing Instructions:
elastic-package install