Move backfill options to start and migrate command

[kvch]

This PR moves the flags backfill-batch-delay and backfill-batch-size to
the commands start and migrate. So these options no longer show up in the list
of global flags.

I created a new struct backfill.Config to store and pass the options down
to the migration runner. I also moved the default values for batch delay and batch size
into the package backfill. I renamed options.go to config.go because it is a better name given the new contents of the file.

Closes #657

[andrew-farries]

I think this has broken the flags - they aren't taking effect for me:

Apply the migration:

{
  "name": "01_create_table",
  "operations": [
    {
      "create_table": {
        "name": "items",
        "columns": [
          {
            "name": "id",
            "type": "serial",
            "pk": true
          },
          {
            "name": "name",
            "type": "text"
          }
        ]
      }
    }
  ]
}

Fill the table with data:

insert into items(name) select 'name_' || i from generate_series(1, 1_000_000) as i

Start a migration with a specified delay:

go run . start --backfill-batch-delay=10s 02_set_unique.json

where the migration is defined:

{
  "name": "02_set_unique",
  "operations": [
    {
      "alter_column": {
        "table": "items",
        "column": "name",
        "unique": {
          "name": "items_name_unique"
        },
        "up": "name",
        "down": "name"
      }
    }
  ]
}

There is no delay between each batch during the backfill.

I think the problem is how the flags are defined as PersistentFlags in the start and migrate commands.

[andrew-farries]

I'd have expected these flags to have moved, not just removed.

I wonder if the tool that generates this CLI description is working correctly.

[andrew-farries]

Not needed I think - appending to a nil slice works fine.

[andrew-farries]

Could remove this and change its only usage to AddCallback instead. Or remove AddCallback and update its usage to WithCallbacks. We don't need both methods IMO.

[andrew-farries]

These don't need to be persistent flags (they don't apply to subcommands, migrate doesn't have any). use .Flags() instead, like the --complete flag.

This might explain why the cli-description.json didn't notice these flags.

[andrew-farries]

Same comment about not needing to use PersistentFlags here as for the migrate command.

[andrew-farries]

The config struct could be embedded:

Suggested change

	config *Config
	*Config

I don't mind either way.

[andrew-farries]

The signature of Start could change to:

func (m *Roll) Start(ctx context.Context, migration *migrations.Migration, c ...*backfill.Config) error

so that the config becomes optional and we don't have to do this everywhere.

It's arguably an anti-pattern though. WDYT?

[kvch]

I am leaning towards passing backfill.NewConfig always. The reason why I introduced the config struct is to avoid passing multiple parameters around in different functions. Changing the signature to varargs defeats the purpose of it. We still need to pass as many parameters as we want to set in the configuration.

[andrew-farries]

The flags still don't work for me, using the same reproduction as in this comment:

#660 (review)

[kvch]

Fixed the issue