Name the S3 keys when storage_url moves to s3:// first (closes #428)

On a config that loads, `config set storage_url s3://...` is refused
until `s3.access_key_id` and `s3.secret_access_key` are set. The refusal
now says to set those two keys first, then `storage_url`.

Model: opus-5-5

internal/cli/config.go

@@ -49,8 +49,14 @@ var (
 	errWouldNotLoad     = errors.New(
 		"vaultik would not load the config after this change, " +
 			"so the file was not changed")
+	errSetS3KeysFirst = errors.New(
+		"set s3.access_key_id and s3.secret_access_key first, " +
+			"then storage_url")
 )
 
+// storageURLKey is the config key that selects the storage backend.
+const storageURLKey = "storage_url"
+
 // macOS is the value of runtime.GOOS on macOS.
 const macOS = "darwin"
 
@@ -513,7 +519,7 @@ func writeConfigSet(out *ui.Writer, path, key, value string) error {
 
 	err = checkConfigChange(oldData, data, settingData)
 	if err != nil {
-		return fmt.Errorf("%w: %w", errWouldNotLoad, err)
+		return wouldNotLoadError(key, err)
 	}
 
 	err = os.WriteFile(path, data, configFileMode)
@@ -537,6 +543,21 @@ func writeConfigSet(out *ui.Writer, path, key, value string) error {
 	return nil
 }
 
+// wouldNotLoadError returns the refusal of a change to key that vaultik
+// would not load for reason. When storage_url moves to s3:// before the S3
+// credentials are set, it says to set them first: they can be set while
+// the old storage_url is still in place.
+func wouldNotLoadError(key string, reason error) error {
+	if key == storageURLKey &&
+		(errors.Is(reason, config.ErrS3KeyIDRequired) ||
+			errors.Is(reason, config.ErrS3SecretRequired)) {
+		return fmt.Errorf("%w: %w; %w", errWouldNotLoad, reason,
+			errSetS3KeysFirst)
+	}
+
+	return fmt.Errorf("%w: %w", errWouldNotLoad, reason)
+}
+
 // checkConfigChange returns the reason vaultik would not load newData, the
 // config after a change, if the change brings that failure in. If oldData,
 // the config before it, loads, the new one must load too. A config that

internal/cli/config_test.go

@@ -2,6 +2,7 @@ package cli //nolint:testpackage // exercises unexported yamlPathGet/yamlPathSet
 
 import (
 	"bytes"
+	"errors"
 	"os"
 	"path/filepath"
 	"strconv"
@@ -152,6 +153,21 @@ snapshots:
       - "~"
 `
 
+// testS3URL is an S3 storage_url, which needs credentials to load.
+const testS3URL = "s3://bucket/prefix?endpoint=https://s3.example.com"
+
+// configWithS3Keys loads: its storage_url is testS3URL and both S3 keys are
+// set.
+const configWithS3Keys = storageURLKey + ": " + testS3URL + `
+s3:
+  access_key_id: AKIAEXAMPLE
+  secret_access_key: example-secret
+snapshots:
+  home:
+    paths:
+      - "~"
+`
+
 // configWithUnsetReference loads only where VAULTIK_TEST_UNSET_SECRET is
 // set, as a backup job's environment might set it. No test sets it.
 const configWithUnsetReference = configWithoutRecipients + `s3:
@@ -258,6 +274,14 @@ func TestConfigSetKeepsFileThatWouldNotLoad(t *testing.T) {
 			"compression_level must be between 1 and 19"},
 		{configWithoutRecipients, "chunk_size", "1KB",
 			"chunk_size must be at least 1MB"},
+		// An s3:// URL before its credentials names them.
+		{configWithoutRecipients, storageURLKey, testS3URL,
+			"s3.access_key_id is required for s3:// URLs; " +
+				"set s3.access_key_id and s3.secret_access_key first"},
+		{configWithoutRecipients + "s3:\n  access_key_id: AKIAEXAMPLE\n",
+			storageURLKey, testS3URL,
+			"s3.secret_access_key is required for s3:// URLs; " +
+				"set s3.access_key_id and s3.secret_access_key first"},
 		// With its reference filled in, the config loads.
 		{levelReference, "blob_size_limit", "1MB",
 			"blob_size_limit must be at least"},
@@ -343,6 +367,76 @@ func TestConfigSetRefusesOnlyItsOwnFailure(t *testing.T) {
 	}
 }
 
+// TestConfigSetS3KeysBeforeStorageURL checks that the order the refusal
+// gives works on a config that loads with a file:// storage_url.
+func TestConfigSetS3KeysBeforeStorageURL(t *testing.T) {
+	t.Parallel()
+
+	path := filepath.Join(t.TempDir(), "config.yml")
+
+	err := os.WriteFile(path, []byte(configWithoutRecipients), configFileMode)
+	if err != nil {
+		t.Fatalf("write config: %v", err)
+	}
+
+	for _, set := range [][2]string{
+		{"s3.access_key_id", "AKIAEXAMPLE"},
+		{"s3.secret_access_key", "example-secret"},
+		{storageURLKey, testS3URL},
+	} {
+		err = writeConfigSet(ui.NewWithColor(&bytes.Buffer{}, false),
+			path, set[0], set[1])
+		if err != nil {
+			t.Fatalf("config set %s: %v", set[0], err)
+		}
+	}
+
+	data, err := os.ReadFile(path) //nolint:gosec // G304: test-controlled path
+	if err != nil {
+		t.Fatalf("read config: %v", err)
+	}
+
+	if !strings.Contains(string(data), storageURLKey+": "+testS3URL) {
+		t.Errorf("config set storage_url wrote:\n%s", data)
+	}
+}
+
+// TestConfigSetRefusesClearingS3Key checks that clearing s3.access_key_id on
+// a config with an s3:// storage_url is refused without saying to set the
+// S3 keys first. That advice is only for a change to storage_url.
+func TestConfigSetRefusesClearingS3Key(t *testing.T) {
+	t.Parallel()
+
+	const key = "s3.access_key_id"
+
+	path := filepath.Join(t.TempDir(), "config.yml")
+
+	err := os.WriteFile(path, []byte(configWithS3Keys), configFileMode)
+	if err != nil {
+		t.Fatalf("write config: %v", err)
+	}
+
+	err = writeConfigSet(ui.NewWithColor(&bytes.Buffer{}, false), path, key, "")
+	if !errors.Is(err, errWouldNotLoad) {
+		t.Errorf("clearing %s gives error %v, want one saying %q",
+			key, err, errWouldNotLoad)
+	}
+
+	if errors.Is(err, errSetS3KeysFirst) {
+		t.Errorf("clearing %s gives error %v, want one not saying %q",
+			key, err, errSetS3KeysFirst)
+	}
+
+	data, err := os.ReadFile(path) //nolint:gosec // G304: test-controlled path
+	if err != nil {
+		t.Fatalf("read config: %v", err)
+	}
+
+	if string(data) != configWithS3Keys {
+		t.Errorf("clearing %s changed the file to:\n%s", key, data)
+	}
+}
+
 // TestConfigSetWritesEnvSection checks that config set writes a key under
 // the top-level env section, whose keys are not settings, on a config that
 // loads.