Add CheckpointListInterface::merge() or ::squash() to allow NOT keeping a checkpoint after application

Naming is tricky here. I think I prefer squash. Also if it is the only checkpoint would a squash be the same as a delete?

Also if it is the only checkpoint would a squash be the same as a delete?

Yes.

Assuming this history:

    $test_data = [
      'hash1' => new Checkpoint('hash1', 'One', 1701539510, NULL),
      'hash2' => new Checkpoint('hash2', 'Two', 1701539520, 'hash1'),
      'hash3' => new Checkpoint('hash3', 'Three', 1701539530, 'hash2'),
    ];
    $state->get(self::CHECKPOINT_KEY, [])->willReturn($test_data);

or visualized: null ← hash1 ← hash2 ← hash3

Then:

1. squash(start: 'hash2', end: 'hash3', new_label: 'Two') yields null ← hash 1 ← hash3
2. squash(start: 'hash1', end: 'hash3', new_label: 'Three') == delete('hash2'), because both yield null ← hash3

IMHO, it might be clearer to ask the calling code to explicitly specify which checkpoints should be condensed. For example, I would find this quite usable, assuming there are 4 checkpoints:

$checkpoint_list->mergeCheckpoints(['checkpoint2', 'checkpoint3'], 'checkpoint4');

(squashCheckpoints() would also be a reasonable name for this method.)

I think this has the advantage of making the intention (and expectations) more explicit. The downside is that you have to remember what the sequence of checkpoints was. For that, though, we could add another method, like $list->listCheckpointsBetween('checkpoint2', 'checkpoint4').

I dunno. Thoughts?

Looking at this from the issue summary:

core/scripts/drupal recipe one --keep-checkpoint
core/scripts/drupal recipe two --no-keep-checkpoint
core/scripts/drupal recipe three --keep-checkpoint

Wouldn't this be, effectively, the same thing?:

core/scripts/drupal recipe one --keep-checkpoint
core/scripts/drupal recipe two --do-not-create-a-checkpoint
core/scripts/drupal recipe three --keep-checkpoint

I feel like that might be easier for us to do. "Always create a checkpoint, unless specifically told not to." Is there any reason that won't work for our purposes?

@phenaproxima Yes there is a reason: @alexpott wants (and I agree) to always create a checkpoint, but then optionally choose to remove it after successfully applying it.

There is a very good reason for always creating a checkpoint: because that ensures we can always revert to a previously good state. It's just that depending on the use case, you may want to keep every checkpoint around, or not.

Note that it doesn't matter if it's a checkpoint for a recipe being depended upon or not: the same need continues to exist when applying a bunch of recipes.

There is a very good reason for always creating a checkpoint: because that ensures we can always revert to a previously good state.

I agree that we should create a checkpoint before applying a recipe that the user specifically triggered.

But for the intermediate (read: hidden) steps? I'm not convinced that someone would want to revert to a "good, but in-between" state -- they'd want to go back to the starting state. The one they knew they were starting from. So I still don't understand why we'd need intermediate checkpoints.

I talked through this with Wim and I'm not convinced this should block #3407956: [PP-1] Create a config storage checkpoint before applying a recipe, and clean it up afterwards → .

That issue, and others in #3405328: [meta] Make recipes safer to use in the real world by supporting config validation and rolling back a broken recipe → , are about safety. Making sure that a broken recipe (or chain of recipes) cannot break your site and leave you out in the cold.

As far as I can tell, this issue, although certainly important from an auditing/cleanliness/clarity perspective, is not related to safety. Without this issue, you can merely end up with a lot of extra intermediate checkpoints that you don't care about. Certainly that is useless cruft, and certainly we should get this done before the recipe system is ready for core...but I'm not convinced it needs to block anything in #3405328: [meta] Make recipes safer to use in the real world by supporting config validation and rolling back a broken recipe → , because it is a convenience issue, not a safety issue, if you have extra, useless checkpoints lying around.

That said, I'm open to being convinced otherwise...but I think this should lose its blocker status.

Discussed with @alexpott in Slack and I think we agreed that this isn't a blocker: https://drupal.slack.com/archives/C2THUBAVA/p1705596762698939

I'm removing the tag, and removing this issue from the meta.