Accept river migrate-down --target-version 0 to remove all River tables (#966)

There was a bit of an unfortunate UX quirk in the CLI wherein to remove
all River tables you needed to specify a special value of
`--target-version -1`. This is a relic of how the CLI uses `rivermigrate`
internally, and `rivermigrate`'s options have a `TargetVersion int` that
makes 0 a reserved value for when the option is unset.

It seems like we can do a little better here for the CLI, so here we add
some code that lets 0 or -1 be used as a `--target-version` to apply all
down migrations. We do this by changing the CLI default value to the
sentinel value -2, which we change back to a real default before
forwarding onto `rivermigrate`.

Author: brandur

CHANGELOG.md

@@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Remove unecessary transactions where a single database operation will do. This reduces the number of subtransactions created which can be an operational benefit it many cases. [PR #950](https://github.com/riverqueue/river/pull/950)
 - Bring all driver tests into separate package so they don't leak dependencies. This removes dependencies from the top level `river` package that most River installations won't need, thereby reducing the transitive dependency load of most River installations. [PR #955](https://github.com/riverqueue/river/pull/955).
+- The River CLI now accepts a `--target-version` of 0 with `river migrate-down` to run all down migrations and remove all River tables (previously, -1 was used for this; -1 still works, but now 0 also works). [PR #966](https://github.com/riverqueue/river/pull/966).
 
 ### Fixed
 

cmd/river/rivercli/river_cli.go

@@ -173,7 +173,7 @@ to use a development database only.
 		cmd.Flags().StringVar(&opts.Line, "line", "", "migration line to operate on (default: main)")
 		cmd.Flags().IntVar(&opts.MaxSteps, "max-steps", 0, "maximum number of steps to migrate")
 		cmd.Flags().BoolVar(&opts.ShowSQL, "show-sql", false, "show SQL of each migration")
-		cmd.Flags().IntVar(&opts.TargetVersion, "target-version", 0, "target version to migrate to (final state includes this version, but none after it)")
+		cmd.Flags().IntVar(&opts.TargetVersion, "target-version", targetVersionCLIFlagDefault, "target version to migrate to (final state includes this version, but none after it); when migrating down, use 0 to apply all down migrations")
 	}
 
 	// migrate-down
@@ -434,7 +434,7 @@ func (c *migrateDown) Run(ctx context.Context, opts *migrateOpts) (bool, error)
 	res, err := migrator.Migrate(ctx, rivermigrate.DirectionDown, &rivermigrate.MigrateOpts{
 		DryRun:        opts.DryRun,
 		MaxSteps:      opts.MaxSteps,
-		TargetVersion: opts.TargetVersion,
+		TargetVersion: targetVersionTranslateDefault(opts.TargetVersion),
 	})
 	if err != nil {
 		return false, err
@@ -645,7 +645,7 @@ func (c *migrateUp) Run(ctx context.Context, opts *migrateOpts) (bool, error) {
 	res, err := migrator.Migrate(ctx, rivermigrate.DirectionUp, &rivermigrate.MigrateOpts{
 		DryRun:        opts.DryRun,
 		MaxSteps:      opts.MaxSteps,
-		TargetVersion: opts.TargetVersion,
+		TargetVersion: targetVersionTranslateDefault(opts.TargetVersion),
 	})
 	if err != nil {
 		return false, err
@@ -721,3 +721,30 @@ func (c *version) Run(ctx context.Context, opts *versionOpts) (bool, error) {
 
 	return true, nil
 }
+
+// A sentinel value used to represent an unset CLI flag for TargetVersion. See
+// comment below for details.
+const targetVersionCLIFlagDefault = -2
+
+// Translates a target version from incoming CLI flag to one that goes in the
+// rivermigrate package's options. The reason this exists is that get around the
+// bad UX required with default values, rivermigrate uses a TargetVersion of 0
+// to do nothing, and a TargetVersion of -1 to specify all versions. This is
+// suboptimal from a CLI perspective, because when migrating down, it requires
+// the caller to know to specify -1 instead of the more natural 0 to down
+// migrate everything.
+//
+// This function translates an incoming CLI option so that a 0 is changed to a
+// -1 so that it migrates everything. -1 stays as is. -2  is used as a default
+// value (targetVersionCLIFlagDefault) for the CLI flags and is changed to 0 so
+// it becomes the default value on the migrate options.
+func targetVersionTranslateDefault(targetVersion int) int {
+	switch targetVersion {
+	case 0:
+		return -1
+	case targetVersionCLIFlagDefault:
+		return 0
+	}
+
+	return targetVersion
+}

cmd/river/rivercli/river_cli_test.go

@@ -556,3 +556,15 @@ func TestRoundDuration(t *testing.T) {
 	require.Equal(t, "34.04s", roundDuration(mustParseDuration("34.042234s")).String())
 	require.Equal(t, "2m34.04s", roundDuration(mustParseDuration("2m34.042234s")).String())
 }
+
+func TestTargetVersion(t *testing.T) {
+	t.Parallel()
+
+	require.Equal(t, 1, targetVersionTranslateDefault(1))
+	require.Equal(t, 2, targetVersionTranslateDefault(2))
+	require.Equal(t, 3, targetVersionTranslateDefault(3))
+
+	require.Equal(t, 0, targetVersionTranslateDefault(-2))
+	require.Equal(t, -1, targetVersionTranslateDefault(-1))
+	require.Equal(t, -1, targetVersionTranslateDefault(0))
+}