feat: add --forceActions option to watch

maxkorp commented 2 years ago

Description

This adds the --forceActions flag to watch, which behaves as it does for the migrate command

Performance impact

If running graphile-migrate watch --once this will run the actions as specified without any real performance hit (except for the time spent to run the actions, obviously), since it's just an additional or statement against a boolean.

TBD: Without the --once I am pretty sure this will run every time you save, perhaps that is desired but it seems potentially unwanted.

Security impact

None that I can think of?

Checklist

[x] My code matches the project's code style and yarn lint:fix passes.
[x] I've added tests for the new feature, and yarn test passes.
[x] I have detailed the new feature in the relevant documentation.
[x] I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
[x] If this is a breaking change I've explained why.

benjie commented 2 years ago

I'm not super happy with the way that this would also force the migration to run in watch mode even if there's no changes; but equally I'm not happy with only running the actions because there might be an action that assumes that the migration has ran freshly (e.g. dropped and recreated a table)...

I wonder if maybe we should add a --forceOnce or similar that both forces the migration and forces the actions? Or maybe we can do --once=force if that's nicer... I suppose --once --forceOnce is harmless. Thoughts?

maxkorp commented 2 years ago

Yeah I didn't love that either. The only time i'd see it being advantageous is testing changes to your dumping script.

What if --forceActions meant that

For watch --once, it forces the actions as expected.
For watch, it forces the actions the first time. So if it needs to run current, it runs current then actions, then it continues on as previously. If it does not need to run current, it runs the actions, then continues on as normal.

benjie commented 2 years ago

That seems reasonable, but I'm concerned someone will come up with a use case for running the actions every time and I'm not sure what we'd then expose that as... --forceActionsEveryTime :wink: I guess we could do --forceActions=always or similar... Though yargs doesn't seem to support --forceActions / --forceActions=foo both being supported?

How about we add graphile-migrate current as basically equivalent to graphile-migrate watch --once, but --forceActions is accepted? That allows us to sidestep --forceActions for watch for now. Also watch --once has always felt wrong, so adding current or similar might be better anyway. Not sure current is the right command though.

maxkorp commented 2 years ago

I like that idea! How about runAll?

benjie commented 2 years ago

I think current is better than runAll, seems clearer what it does to me at least.

maxkorp commented 2 years ago

Working on this now, would love to talk about what current does in terms of which actions it runs when, in order to remain consistent

I imagine like so? (Please forgive my comical verbosity here)

Committed Migrations are run, no changes to current, no force

doesnt run any committed since they are complete
doesn't run afterAllMigrations actions since we didn't run any migrations
doesn't run current since there are no changes
doesn't run afterCurrent since it didn't run current

Committed Migrations are run, no changes to current, forceActions passed

doesnt run any committed since they are complete
does run afterAllMigrations actions since we passed forceActions
doesn't run current since there are no changes
does run afterCurrent since we passed forceActions

Committed Migrations are run, changes to current, no force

doesnt run any committed since they are complete
doesn't run afterAllMigrations actions since we didn't run any migrations
does run current since there are changes
does run afterCurrent since it did run current

Committed Migrations are run, changes to current, forceActions passed

doesnt run any committed since they are complete
does run afterAllMigrations actions since we passed forceActions
does run current since there are changes
does run afterCurrent since it did run current (and we passed forceActions)

Committed Migrations are not run, no changes to current, no force

Is this state possible? or does it always run current if it ran some committed migrations, even if it's empty? Like in the case of watch --once I mean.

does run committed migrations since they are not all run yes
does run afterAllMigrations actions since we ran migrations
doesn't run current since there are no changes
doesn't run afterCurrent since it didn't run current

Committed Migrations are not run, no changes to current, forceActionsPassed

See above question

does run committed migrations since they are not all run
does run afterAllMigrations actions since we ran migrations (and we passed forceActions)
doesn't run current since there are no changes
does run afterCurrent since we passed forceActions

Committed Migrations are not run, changes to current, no force

Also Committed Migrations are not run, changes to current, forceActionsPassed

These are the same since the actions are run anyways, yes?

does run committed migrations since they are not all run
does run afterAllMigrations actions since we ran migrations (and we passed forceActions, if we did)
does run current since there are changes
does run afterCurrent since it did run current (and we passed forceActions, if we did)

Like, this is the fully broken out tree. Obviously we can simplify this down a bit, I just want to be super explicit about which cases I'm not sure about.

If forceActions is passed, we always run afterAllMigrations and afterCurrent no matter what, but if we don't, I'm unsure what we run in the case of there being committed migrations to run, but nothing in current.

I was going to pitch an afterAnyMigrations or something of the sort since I might want to run a script (the dump in my case) after running everything, but only once (right now we have it in both afterAllMigrations and afterCurrent which is slow), but I suppose by stuffing it in only afterCurrent and using forceActions that is handled anyways.

maxkorp commented 2 years ago

Side note, I'll squish this down once we're ready with it, I just want to maintain what I've done while still working on it.

maxkorp commented 2 years ago

Also, I'm not 100% sure that I love that we pull the "running current script" code from watch into current. Perhaps that belongs in current, and watch pulls it in (basically just reversing the direction on that import/export.

graphile / migrate