Default behavior of sqitch revert is too dangerous.

[vectro]

It makes sense that sqitch deploy would deploy all the changes. But the default behavior of sqitch revert — reverting all changes — is super dangerous. That is because in the typical case of adding columns or tables, revert scripts cause data loss while deploy scripts don't.

I would suggest the following changes:

• Don't revert all changes by default; instead add a new command line parameter --revert-all, --revert-all-changes, --all-changes, or similar to achieve this behavior.
• Provide a new command line parameter to specify the exact change or changes to be reverted, which would error out if there are dependent changes already deployed. This is the safest way to revert, when only that which is explicitly requested is reverted.

[theory]

Hi @vectro, thanks for your suggestion. A couple things:

• You can change the behavior of the automatic reversion on deploy failure by setting deploy.mode configuration to tag to revert to the last tag by default, or change to revert only the last change. In my experience revert is almost never called in production, but the automatic reversion configuration can be important to prevent unexpected reversions when a deploy fails.
• You can specify the exact change to revert to! In the docs, that's the <change> argument, and there are a lot of ways to specify a change, as described in sqitch-changes. I use this often while doing development to revert to @HEAD^ --- the second-to-last deployed change. In fact, I most often use sqitch rebase @HEAD^ as I iterate on the development of a new change.

[vectro]

Thanks @theory. I think the reality that "revert is almost never called in production" is the source of my anxiety; because it's not a normal thing to do, I worry that someone would do it by accident, for example trying to understand what the command does.

Regarding the second bullet point, you can specify which change to revert to, but not which change to revert. The former is probably more convenient but the latter is safer because it describes exactly which action(s) would be run.

[theory]

In the ten years people have been using Sqitch, I have not heard of a case of someone accidentally reverting in production. Of course it can happen, which is why most people set revert.prompt_accept to false, so that when someone tries to revert, it prompts them to be sure and the default answer is "no".

Sqitch requires that changes be deployed and reverted in order; it does not support reverting arbitrary changes in the plan. You can think of it as each change in the plan implicitly requires all the changes before it. This feature is described in the about page:

Deployment integrity

Sqitch manages changes and dependencies via a plan file, and employs a Merkle tree pattern similar to Git and Blockchain to ensure deployment integrity. As such, there is no need to number your changes, although you can if you want. Sqitch doesn’t much care how you name your changes.

[vectro]

Thanks @theory. Perhaps revert.prompt_accept=false should be the default?

Regarding reverting to a specific change, maybe it's best to move that to a separate issue to keep the conversation more focused. I have opened #661 for that.

[vectro]

Thinking about this further, it is still scary to me to think that someone could run sqitch revert with no changes and accidentally break things in a pretty bad way. Although I would hope anyone with elevated database access would not answer "yes" to a question "Revert all changes?", people do make mistakes, especially when things are broken, and it's best to design systems in such a way so that mistakes are hard to make.

What do you think about adding an additional option revert.change_mandatory that would disallow the "Revert all changes" behavior? I am happy to submit a patch.

[vectro]

I suppose another similar option would be a "paranoid mode" configuration option that would:

• Require confirmation for revert and deploy
• Require specifying a change for revert
• Disable checkout and rebase

What do you think?

[theory]

If you set deploy.mode to tag and only deploy changes up to a tag in to production, sqitch revert should do nothing if the most recent deployed change is tagged.

[vectro]

That is not what I observe in testing, nor does the source code look to me like it is checking anything like that. Have I misunderstood?

[theory]

Ah, I am misremembering, as this comment illustrates:

sqitch/lib/App/Sqitch/Engine.pm

Lines 357 to 361 in ef7767d

	# Do we want to support modes, where failures would re-deploy to previous
	# tag or all the way back to the starting point? This would be very much
	# like deploy() mode. I'm thinking not, as a failure on a revert is not
	# something you generally want to recover from by deploying back to where
	# you started. But maybe I'm wrong?

Turns out modes only affect where one automatically reverts to in the event of a deploy failure. Apologies for the error.

I have no particular objection to a kind of strict mode for reverts if that's something you'd like to work on. Would recommend to avoid terms having to do with mental health, though, like "paranoia"; Something like "strict" or "explicit" would be a better choice, methinks.

[vectro]

Opened #719 for this.

[theory]

Closing with the merging of #735.