diff --git a/cmd/engine_mock_test.go b/cmd/engine_mock_test.go index f4ff98d..8f251ed 100644 --- a/cmd/engine_mock_test.go +++ b/cmd/engine_mock_test.go @@ -18,6 +18,8 @@ type mockEngine struct { containerExists map[string]bool containerExistsErr error + imageExists bool + volumes map[string]bool volumeExistsErr error createVolumeErr error @@ -88,6 +90,8 @@ func (m *mockEngine) RemoveVolume(name string) error { } func (m *mockEngine) ExportVolume(string, io.Writer) error { return m.exportVolumeErr } +func (m *mockEngine) ImageExists(string) (bool, error) { return m.imageExists, nil } + func (m *mockEngine) PullImage(_ string, msgs chan<- string) error { if msgs != nil { for _, line := range m.pullMsgs { diff --git a/cmd/root.go b/cmd/root.go index 0ba85c8..e9f61da 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -2,6 +2,7 @@ package cmd import ( "context" + "errors" "fmt" "os" "os/signal" @@ -46,6 +47,15 @@ var ( Run omnideck without a command. It will open the right screen for first setup, repair, or managing an existing installation.`, PersistentPreRun: persistentPreRun, + // A failing command has already said what went wrong, in the shape the + // caller asked for. Printing the whole usage block after it buries that + // message, and under --json it puts a wall of prose on stderr behind a + // single line of JSON. + SilenceUsage: true, + // Errors are printed by Execute instead, so the one error that carries + // no message can exit quietly rather than leaving a bare "Error:" + // behind the output the caller actually asked for. + SilenceErrors: true, } ) @@ -65,6 +75,9 @@ func Execute() { defer stop() engine.SetCancelContext(ctx) if err := rootCmd.Execute(); err != nil { + if !errors.Is(err, errAborted) { + fmt.Fprintln(os.Stderr, "Error:", err) + } os.Exit(1) } } diff --git a/engine/docker.go b/engine/docker.go index 04860a5..d9c24f9 100644 --- a/engine/docker.go +++ b/engine/docker.go @@ -44,6 +44,15 @@ func (e *DockerEngine) ContainerExists(name string) (bool, error) { return strings.TrimSpace(string(out)) == name, nil } +func (e *DockerEngine) ImageExists(image string) (bool, error) { + // Docker has no "image exists"; inspecting one that is absent fails. + cmd := buildCmd("docker", "image", "inspect", image) + if err := cmd.Run(); err != nil { + return false, nil + } + return true, nil +} + func (e *DockerEngine) CreateVolume(name string) error { cmd := buildCmd("docker", "volume", "create", name) out, err := cmd.CombinedOutput() diff --git a/engine/engine.go b/engine/engine.go index ff18368..612c019 100644 --- a/engine/engine.go +++ b/engine/engine.go @@ -43,6 +43,10 @@ type Engine interface { RemoveVolume(name string) error ExportVolume(name string, w io.Writer) error PullImage(image string, msgs chan<- string) error + // ImageExists reports whether the image is already on this machine, so a + // registry that cannot be reached need not stop work that has everything + // it needs locally. + ImageExists(image string) (bool, error) RunContainer(opts RunOptions) error // CheckOllamaConnection tests the same container-to-host address Omnideck // receives. It runs from inside an existing Omnideck container. diff --git a/engine/podman.go b/engine/podman.go index 277c58c..2f0d29a 100644 --- a/engine/podman.go +++ b/engine/podman.go @@ -35,6 +35,15 @@ func (e *PodmanEngine) ContainerExists(name string) (bool, error) { return strings.TrimSpace(string(out)) == name, nil } +func (e *PodmanEngine) ImageExists(image string) (bool, error) { + cmd := buildCmd("podman", "image", "exists", image) + if err := cmd.Run(); err != nil { + // Absent is reported by exit status, and is not a failure to ask. + return false, nil + } + return true, nil +} + func (e *PodmanEngine) CreateVolume(name string) error { return createVolumeIfMissing(name, e.VolumeExists, func() error { cmd := buildCmd("podman", "volume", "create", name) diff --git a/tui/test_helpers_test.go b/tui/test_helpers_test.go index c7734c6..7e2a6a4 100644 --- a/tui/test_helpers_test.go +++ b/tui/test_helpers_test.go @@ -43,6 +43,7 @@ func (m *mockEngine) IsAvailable() bool { return true } func (m *mockEngine) HasPermission() bool { return true } func (m *mockEngine) Version() string { return "1.0" } func (m *mockEngine) ImageDigest(string) string { return "" } +func (m *mockEngine) ImageExists(string) (bool, error) { return true, nil } func (m *mockEngine) PullImage(string, chan<- string) error { return nil } func (m *mockEngine) RunContainer(opts engine.RunOptions) error { m.lastRunOptions = opts diff --git a/workflow/create_instance.go b/workflow/create_instance.go index a37174b..3ff0ea8 100644 --- a/workflow/create_instance.go +++ b/workflow/create_instance.go @@ -16,6 +16,7 @@ type InstanceCreationEngine interface { CreateVolume(name string) error RemoveVolume(name string) error PullImage(image string, msgs chan<- string) error + ImageExists(image string) (bool, error) RunContainer(opts engine.RunOptions) error RemoveContainer(name string) error } @@ -109,7 +110,17 @@ func CreateInstance(eng InstanceCreationEngine, cfg *config.Config, save func() close(msgs) <-forwarded if pullErr != nil { - return pullErr + // A registry that cannot be reached is only fatal when the image is not + // already here. Setting up again offline, or repairing an installation + // whose container is gone, needs nothing downloaded — and an image + // named by digest cannot have changed since it was fetched. + present, existsErr := eng.ImageExists(cfg.Image) + if existsErr != nil || !present { + return pullErr + } + if opts.OnPullProgress != nil { + opts.OnPullProgress("Using the copy already on this computer") + } } opts.reportStage("run_container") diff --git a/workflow/create_instance_test.go b/workflow/create_instance_test.go index 8adccf8..32603a9 100644 --- a/workflow/create_instance_test.go +++ b/workflow/create_instance_test.go @@ -3,6 +3,7 @@ package workflow import ( "context" "errors" + "strings" "testing" "github.com/omnideck-dev/cli/config" @@ -16,11 +17,16 @@ type fakeCreationEngine struct { createdVolumes []string removedVolumes []string pullErr error + imagePresent bool pullMsgs []string runErr error removedContaner bool } +func (f *fakeCreationEngine) ImageExists(string) (bool, error) { + return f.imagePresent, nil +} + func (f *fakeCreationEngine) ContainerExists(string) (bool, error) { return f.containerExists, f.existsErr } @@ -179,3 +185,38 @@ func TestCreateInstanceFailureDuringCancellationReportsContextCanceled(t *testin t.Fatalf("removedVolumes = %v, want both volumes cleaned up despite cancellation", eng.removedVolumes) } } + +func TestCreateInstanceUsesTheLocalImageWhenTheRegistryCannotBeReached(t *testing.T) { + // Setting up again offline, or repairing an installation whose container is + // gone, needs nothing downloaded. An image named by digest cannot have + // changed since it was fetched, so the copy already here is the right one. + eng := &fakeCreationEngine{pullErr: errors.New("pinging container registry: Forbidden"), imagePresent: true} + cfg := testCreateConfig() + + var progress []string + err := CreateInstance(eng, cfg, func() error { return nil }, CreateInstanceOptions{ + OnPullProgress: func(line string) { progress = append(progress, line) }, + }) + if err != nil { + t.Fatalf("expected the local image to be used, got %v", err) + } + if len(eng.removedVolumes) != 0 || eng.removedContaner { + t.Fatal("nothing failed, so nothing should have been cleaned up") + } + if len(progress) == 0 || !strings.Contains(progress[len(progress)-1], "already on this computer") { + t.Fatalf("expected the fallback to be reported, got %v", progress) + } +} + +func TestCreateInstanceFailsWhenTheImageIsNeitherFetchableNorPresent(t *testing.T) { + eng := &fakeCreationEngine{pullErr: errors.New("pinging container registry: Forbidden")} + cfg := testCreateConfig() + + err := CreateInstance(eng, cfg, func() error { return nil }, CreateInstanceOptions{}) + if err == nil { + t.Fatal("expected the unreachable registry to fail when nothing is here to use") + } + if len(eng.removedVolumes) != 2 { + t.Fatalf("removedVolumes = %v, want both volumes cleaned up", eng.removedVolumes) + } +}