Update backend formatting and add --format flag

[amoses12]

This PR adds a new --format flag and adds information about the deprecation of the existing --mount-format flag. It also moves all of our formatting to the backend for parity across commands and features.

[emily-curry]

blocking: It is pretty unfortunate that we're introducing a regression in functionality here, where now mounting to an env file (or other formats) can no longer use the fallback file or cache. I was hoping that we'd unwind some of the internals of FetchSecrets, it really should just return a raw byte array, since it accepts format as an argument. All FetchSecrets does with it internally is some validations before writing the fallback file, I'm not convinced those are necessary.

If we're not going to do the work to keep the same level of functionality, we still should not be passing around an empty map and putting the raw bytes somewhere else, that's super hacky. At the very least, secrets here should be a byte array, ValidateSecrets can be moved closer to FetchSecrets, and PrepareSecrets should accept a byte array.

Finally, we should get rid of all of the dead code. SecretsToBytes can be removed entirely. secrets_mount.go can also be removed entirely (though the template option will need to be accommodated elsewhere. we should not introduce a breaking change, but this is not a "real" format, it's json piped through a user-defined local template file).

[amoses12]

I made some major refactors in the last force push. Please let me know if this is closer to what we're looking for.

[emily-curry]

Looks great! The tests still need fixed, but I'm very happy to see a solution that makes the caching/fallback file behavior work for all formats.

[emily-curry]

nit: It doesn't look like SecretsCacheFile is in use anymore.

[emily-curry]

nit: Consolidate this into the if statement above?

[emily-curry]

blocking: We do need to fix the failing e2e tests, as some of the behaviors have changed. I wrote a few just to test some of the behaviors myself.

One of these tests fails on an edge case: If you mount/download once with a fallback file in one format, then mount/download again with a fallback file in a different format and provide --fallback-only (so all operations are fully offline), you get the data from the first format, when you asked for it in the second format. I'm including that test here to demonstrate the issue, but I would just print a warning to the effect of "--format is ignored when --fallback-only is provided" and remove the failing test. If you're asking for the operation to be fully offline and you have a fallback file, you probably just expect whatever's in that fallback file.

diff --git a/tests/e2e/run-mount.sh b/tests/e2e/run-mount.sh
index 2c10e29..1cee46d 100755
--- a/tests/e2e/run-mount.sh
+++ b/tests/e2e/run-mount.sh
@@ -75,6 +75,38 @@ fi
 
 beforeEach
 
+# verify fallback file can be used
+EXPECTED_SECRETS='DOPPLER_CONFIG="e2e"\nDOPPLER_ENVIRONMENT="e2e"\nDOPPLER_PROJECT="cli"\nFOO="bar"\nHOME="123"\nTEST="abc"'
+"$DOPPLER_BINARY" run --mount secrets.env --fallback ./fallback.txt --command "cat \$DOPPLER_CLI_SECRETS_PATH" > /dev/null
+actual="$("$DOPPLER_BINARY" run --mount secrets.env --fallback ./fallback.txt --fallback-only --command "cat \$DOPPLER_CLI_SECRETS_PATH")"
+rm -f ./fallback.txt
+if [[ "$actual" != "$(echo -e "$EXPECTED_SECRETS")" ]]; then
+ echo "ERROR: mounted secrets file with env format has invalid contents"
+ exit 1
+fi
+
+beforeEach
+
+# verify attempting to mount with mismatched format and --fallback-only fails
+"$DOPPLER_BINARY" run --mount secrets.env --format env --fallback ./fallback.txt --command "cat \$DOPPLER_CLI_SECRETS_PATH" > /dev/null
+"$DOPPLER_BINARY" run --mount secrets.json --format json --fallback ./fallback.txt --fallback-only --command "cat \$DOPPLER_CLI_SECRETS_PATH" > /dev/null && \
+  (echo "ERROR: should not be able to use fallback file written with env format to mount json" && rm -f ./fallback.txt && exit 1)
+rm -f ./fallback.txt
+
+beforeEach
+
+# verify attempting to mount with mismatched format without --fallback-only gracefully handles mismatch and fetches correct format
+EXPECTED_SECRETS='{"DOPPLER_CONFIG":"e2e","DOPPLER_ENVIRONMENT":"e2e","DOPPLER_PROJECT":"cli","FOO":"bar","HOME":"123","TEST":"abc"}'
+"$DOPPLER_BINARY" run --mount secrets.env --format env --fallback ./fallback.txt --command "cat \$DOPPLER_CLI_SECRETS_PATH" > /dev/null
+actual="$("$DOPPLER_BINARY" run --mount secrets.json --format json --fallback ./fallback.txt --command "cat \$DOPPLER_CLI_SECRETS_PATH")"
+rm -f ./fallback.txt
+if [[ "$actual" != "$(echo -e "$EXPECTED_SECRETS")" ]]; then
+ echo "ERROR: mounted secrets file with json format has invalid contents"
+ exit 1
+fi
+
+beforeEach
+
 # verify specified format is used
 EXPECTED_SECRETS='{"DOPPLER_CONFIG":"e2e","DOPPLER_ENVIRONMENT":"e2e","DOPPLER_PROJECT":"cli","FOO":"bar","HOME":"123","TEST":"abc"}'
 actual="$("$DOPPLER_BINARY" run --mount secrets.env --mount-format json --command "cat \$DOPPLER_CLI_SECRETS_PATH")"
diff --git a/tests/e2e/secrets-download-fallback.sh b/tests/e2e/secrets-download-fallback.sh
index 72612bc..b20cea9 100755
--- a/tests/e2e/secrets-download-fallback.sh
+++ b/tests/e2e/secrets-download-fallback.sh
@@ -49,10 +49,10 @@ test_fallback_dir="$test_config_dir/fallback"
 mkdir -p "$test_fallback_dir"
 DOPPLER_CONFIG_DIR="$test_config_dir" "$DOPPLER_BINARY" secrets download --no-file > /dev/null
 
-fallback_file_count="$(find "$test_fallback_dir" -name '.secrets-*' | wc -l)"
+fallback_file_count="$(find "$test_fallback_dir" -name '.secrets-*' | wc -l | awk '{$1=$1};1')"
 [[ "$fallback_file_count" == "1" ]] || (echo "ERROR: 'run' did not create fallback file in DOPPLER_CONFIG_DIR/fallback" && exit 1)
 
-metadata_file_count="$(find "$test_fallback_dir" -name '.metadata-*' | wc -l)"
+metadata_file_count="$(find "$test_fallback_dir" -name '.metadata-*' | wc -l | awk '{$1=$1};1')"
 [[ "$metadata_file_count" == "1" ]] || (echo "ERROR: 'run' did not create metadata file in DOPPLER_CONFIG_DIR/fallback" && exit 1)
 
 beforeEach
@@ -157,14 +157,11 @@ rm -f ./doppler.env
 
 beforeEach
 
-# test 'secrets download' doesn't write fallback when format is env
-"$DOPPLER_BINARY" secrets download --no-file --format=env > /dev/null
-"$DOPPLER_BINARY" secrets download --no-file --fallback-only > /dev/null 2>&1 && (echo "ERROR: 'secrets download' should not write fallback file when format is env" && exit 1)
-
-beforeEach
-
-# test 'secrets download' ignores fallback flags when format is env
-"$DOPPLER_BINARY" secrets download --no-file --fallback-only --fallback=./nonexistent-file --format=env > /dev/null
+# test 'secrets download' writes fallback when format is env
+a="$("$DOPPLER_BINARY" secrets download --no-file --format=env --fallback ./fallback.txt)"
+b="$("$DOPPLER_BINARY" secrets download --no-file --fallback-only --fallback ./fallback.txt)"
+rm -f fallback.txt
+[[ "$a" == "$b" ]] || (echo "ERROR: 'secrets download' file contents do not match when used as fallback file for 'secrets download'" && exit 1)
 
 beforeEach
 
@@ -175,25 +172,19 @@ rm -f ./secrets.yaml
 
 beforeEach
 
-# test 'secrets download' doesn't write fallback when format is yaml
-"$DOPPLER_BINARY" secrets download --no-file --format=yaml > /dev/null
-"$DOPPLER_BINARY" secrets download --no-file --fallback-only > /dev/null 2>&1 && (echo "ERROR: 'secrets download' should not write fallback file when format is yaml" && exit 1)
-
-beforeEach
-
-# test 'secrets download' ignores fallback flags when format is yaml
-"$DOPPLER_BINARY" secrets download --no-file --fallback-only --fallback=./nonexistent-file --format=yaml > /dev/null
-
-beforeEach
-
-# test 'secrets download' doesn't write fallback when format is docker
-"$DOPPLER_BINARY" secrets download --no-file --format=docker > /dev/null
-"$DOPPLER_BINARY" secrets download --no-file --fallback-only > /dev/null 2>&1 && (echo "ERROR: 'secrets download' should not write fallback file when format is docker" && exit 1)
+# test 'secrets download' writes fallback when format is yaml
+a="$("$DOPPLER_BINARY" secrets download --no-file --format=yaml --fallback ./fallback.txt)"
+b="$("$DOPPLER_BINARY" secrets download --no-file --fallback-only --fallback ./fallback.txt)"
+rm -f fallback.txt
+[[ "$a" == "$b" ]] || (echo "ERROR: 'secrets download' file contents do not match when used as fallback file for 'secrets download'" && exit 1)
 
 beforeEach
 
-# test 'secrets download' ignores fallback flags when format is docker
-"$DOPPLER_BINARY" secrets download --no-file --fallback-only --fallback=./nonexistent-file --format=docker > /dev/null
+# test 'secrets download' writes fallback when format is docker
+a="$("$DOPPLER_BINARY" secrets download --no-file --format=docker --fallback ./fallback.txt)"
+b="$("$DOPPLER_BINARY" secrets download --no-file --fallback-only --fallback ./fallback.txt)"
+rm -f fallback.txt
+[[ "$a" == "$b" ]] || (echo "ERROR: 'secrets download' file contents do not match when used as fallback file for 'secrets download'" && exit 1)
 
 beforeEach

[amoses12]

@emily-curry all of your outstanding comments have been addressed in the latest force push. I ran the e2e tests locally and they all seem to pass. Thanks for the thorough review!

[emily-curry]

The e2e test files touched here have different permissions, they're missing the execute bit. Please make sure these are passing in CI.