Add a new strict-mode configuration option #719

vectro · 2023-02-28T00:27:42Z

This new configuration option disables some functionality that could be perceived as creating risks to production environments.

To begin with, that includes:

The checkout and rebase commands
Running revert without a change specified ("Revert All Changes" function).

lib/App/Sqitch.pm

t/rebase.t

lib/sqitch-config.pod

lib/App/Sqitch/Role/RevertDeployCommand.pm

theory · 2023-03-05T21:15:09Z

lib/sqitch-config.pod

@@ -484,6 +484,25 @@ by C<core.top_dir>.
 The file name extension on deploy, revert, and verify SQL scripts. Defaults to
 C<sql>.

+=item C<core.strict>


Appreciate the docs here, but I think the option and config need to be documented for every command they apply to. Should include, at least, these files, I think:

lib/sqitch-deploy.pod lib/sqitch-revert-usage.pod lib/sqitch-rebase.pod lib/sqitch-revert.pod lib/sqitch-deploy-usage.pod lib/sqitch-checkout-usage.pod lib/sqitch-checkout.pod lib/sqitch-rebase-usage.pod lib/App/Sqitch/Command/revert.pm lib/App/Sqitch/Command/deploy.pm

The main pod files for each command also include a "Configuration Variables" that should have the strict mode documented -- anywhere the string no_prompt appears.

I'm happy to extend the documentation of course, but in some of the cases that you listed I'm not sure what I would say.

sqitch-deploy.pod, sqitch-deploy-usage.pod, and deploy.pm: I don't think this change affects the deploy command at all?

sqitch-deploy-usage.pod, sqitch-checkout-usage.pod, sqitch-revert-usage.pod, and sqitch-rebase-usage.pod: These files don't seem to currently describe configuration options at all. Are you proposing to add new sections?

deploy.pm and revert.pm: It looks to me like these are only documenting the attributes, not the configuration, is that wrong? I don't think this PR changes any of the attributes for these two modules.

The main pod files for each command also include a "Configuration Variables" that should have the strict mode documented -- anywhere the string no_prompt appears.

I think this is covered by your suggestions above, but let me know if you disagree.

I will push a change to sqitch-rebase.pod, sqitch-revert.pod, sqitch-checkout.pod in the meantime.

Right, not deploy, sorry.

I see options listed in the -usage.pod files; new ones like --strict that apply to each command should be added (this is related to my other request not to make it a global option I guess).

Yeah, if you move the option to revert.pm it would then need to be documented.

Oh, I don't think we are adding new options here, only new configuration variables.

Unless there is something which I don't understand that automatically creates options from configuration settings?

Oh, right. In that case it should probably follow the cascading configuration pattern of

--strict

$cmd.strict

target.$name.strict

engine.$name.strict

core.strict

Most of that resolution is done in App::Sqitch::Target, using the _fetch method:

sqitch/lib/App/Sqitch/Target.pm

Lines 76 to 84 in aace1e8

sub _fetch {

my ($self, $key) = @_;

my $config = $self->_config;

return $config->get( key => "target." . $self->name . ".$key" )

|| do {

my $ekey = $self->engine_key;

$ekey ? $config->get( key => "engine.$ekey.$key") : ();

} || $config->get( key => "core.$key");

}

I think it'd be fine to do so in a subsequent PR.

lib/App/Sqitch/Command/revert.pm

theory · 2023-03-19T22:44:17Z

lib/sqitch-config.pod

@@ -484,6 +484,25 @@ by C<core.top_dir>.
 The file name extension on deploy, revert, and verify SQL scripts. Defaults to
 C<sql>.

+=item C<core.strict>


Oh, right. In that case it should probably follow the cascading configuration pattern of

--strict

$cmd.strict

target.$name.strict

engine.$name.strict

core.strict

Most of that resolution is done in App::Sqitch::Target, using the _fetch method:

sqitch/lib/App/Sqitch/Target.pm

Lines 76 to 84 in aace1e8

sub _fetch {

my ($self, $key) = @_;

my $config = $self->_config;

return $config->get( key => "target." . $self->name . ".$key" )

|| do {

my $ekey = $self->engine_key;

$ekey ? $config->get( key => "engine.$ekey.$key") : ();

} || $config->get( key => "core.$key");

}

I think it'd be fine to do so in a subsequent PR.

theory

Okay, did some thinking through of this proposal again, and here's where I'm at:

Like no_prompt, strict should be configured as a revert variable only, revert.strict. And just as no_prompt can be set by -y, so too can strict be set by --strict, but also by --no-strict. There is no need for core.strict because it applies to no other actions other than revert.
Why not allow the rebase command to succeed when strict is enabled as long as to_change is set?
The checkout command always has an implicit target: the most recent common change between the current and target branches. Why disable it?
The --strict option must be documented for every command where it's relevant. Based on the -y option, that's:
- lib/sqitch-checkout-usage.pod
- lib/sqitch-checkout.pod
- lib/sqitch-rebase-usage.pod
- lib/sqitch-rebase.pod
- lib/sqitch-revert-usage.pod
- lib/sqitch-revert.pod
Likewise, the revert.strict config variable should be documented anywhere revert.no_prompt is documented, which appears to be:
- lib/sqitch-rebase.pod
- lib/sqitch-revert.pod
- lib/sqitch-checkout.pod
- lib/App/Sqitch/Role/RevertDeployCommand.pm
- lib/App/Sqitch/Command/revert.pm

Depending on where we come down on the affect on rebase and checkout, we may want to also add support for rebase.strict and checkout.strict, again just like no_prompt, but that may be excessive.

What do you think?

vectro · 2023-03-24T21:03:41Z

Hi David,

Do you mind saying more about the use case that you imagine for the --strict and --no-strict command line options?

In my mind, the purpose of the strict mode is to prevent the accidental use of potentially dangerous features, either by users who are not familiar with Sqitch, or by users who might be familiar with these commands but targeting the wrong environment. In other words, it's to protect users from accidentally doing the wrong thing.

From that perspective I don't understand the situation where someone would want to enable or disable this on the command line. If you want the strict behavior, I would think you especially want it in situations where people didn't think to ask for it.

Ian

theory · 2023-03-24T21:17:43Z

Ah yes, I was working off the assumption that almost every configuration in Sqitch also has an option; overlooked that your PR does not.

In that case, though, I think it makes sense to follow the prompt_accept configuration, which does not have an equivalent option, and is set via revert.prompt_accept' (no core.prompt_accept), and overridden by checkout.prompt_acceptorrebase.prompt_accept`.

It might be useful to extend these two options to targets, too, as I mentioned earlier, but not necessary here.

vectro · 2023-03-27T22:19:47Z

It might be useful to extend these two options to targets

I agree, but maybe we can do that in a subsequent MR?

theory · 2023-03-27T22:23:16Z

I agree, but maybe we can do that in a subsequent MR?

Yes, absolutely.

vectro · 2023-03-28T23:06:51Z

Why not allow the rebase command to succeed when strict is enabled as long as to_change is set?

I guess my belief is that this command is good for rapid development but that when it comes to production deployments (where reverting can cause data loss) it would be better to be more explicit about what changes should be reverted. The whole idea of reverting and then re-deploying seems to me like an exceptional workflow in a production environment — even though it could happen from time to time, it's better to be explicit about it.

(Assuming you meant onto-change, let me know if I misunderstood the suggestion)

vectro · 2023-03-28T23:08:40Z

The checkout command always has an implicit target: the most recent common change between the current and target branches. Why disable it?

It's less about the possibility that all changes would be reverted and more about the possibility than anything could be reverted without the operator having the specific intention to revert those specific changes. In other words, "implied target change" is probably usually better than "revert everything", but still risky.

functionality that could be perceived as creating risks to production environments.

vectro · 2023-03-28T23:09:58Z

@theory What do you think about this latest attempt?

theory · 2023-04-01T14:48:33Z

(Assuming you meant onto-change, let me know if I misunderstood the suggestion)

Yes, that's what I meant, was thinking about it from the POV of consistency. This is part of why I've been thinking it'd be useful to support the option at the target level, as you would want strict enabled for a prod target but not a dev target. That said, we can leave it as revert.strict and the disabling behavior, and recommend that it be used at the --user or --system config level. You'd enable it on whatever box was responsible for production reverts.

theory · 2023-04-01T14:52:54Z

@theory What do you think about this latest attempt?

Looks good, I think it's almost there. I'll pull it down and give it a through review this weekend. Thanks!

theory · 2023-04-03T01:46:33Z

Have a look at #735, @vectro. If it looks good to you I'll merge it (which will also merge this PR). Thanks!

theory · 2023-04-10T01:54:36Z

Closing in favor of #735, which includes your commit.

theory requested changes Mar 5, 2023

View reviewed changes

vectro force-pushed the strict branch from d32ed1e to 39ac702 Compare March 7, 2023 20:25

vectro force-pushed the strict branch from 39ac702 to 54ce1d4 Compare March 16, 2023 23:19

theory previously approved these changes Mar 19, 2023

View reviewed changes

vectro dismissed theory’s stale review via 479b187 March 21, 2023 18:25

vectro force-pushed the strict branch from 54ce1d4 to 479b187 Compare March 21, 2023 18:25

theory reviewed Mar 24, 2023

View reviewed changes

Add a new strict-mode configuration option, which disables some

d7bafe6

functionality that could be perceived as creating risks to production environments.

vectro force-pushed the strict branch from 479b187 to d7bafe6 Compare March 28, 2023 23:09

theory mentioned this pull request Apr 3, 2023

Refactor strict mode #735

Merged

theory closed this Apr 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a new strict-mode configuration option #719

Add a new strict-mode configuration option #719

vectro commented Feb 28, 2023

theory Mar 5, 2023

vectro Mar 7, 2023

theory Mar 11, 2023

vectro Mar 13, 2023

theory Mar 19, 2023

theory Mar 19, 2023

theory left a comment

vectro commented Mar 24, 2023

theory commented Mar 24, 2023

vectro commented Mar 27, 2023

theory commented Mar 27, 2023

vectro commented Mar 28, 2023

vectro commented Mar 28, 2023

vectro commented Mar 28, 2023

theory commented Apr 1, 2023

theory commented Apr 1, 2023

theory commented Apr 3, 2023

theory commented Apr 10, 2023

	sub _fetch {
	my ($self, $key) = @_;
	my $config = $self->_config;
	return $config->get( key => "target." . $self->name . ".$key" )
	\|\| do {
	my $ekey = $self->engine_key;
	$ekey ? $config->get( key => "engine.$ekey.$key") : ();
	} \|\| $config->get( key => "core.$key");
	}

Add a new strict-mode configuration option #719

Add a new strict-mode configuration option #719

Conversation

vectro commented Feb 28, 2023

theory Mar 5, 2023

Choose a reason for hiding this comment

vectro Mar 7, 2023

Choose a reason for hiding this comment

theory Mar 11, 2023

Choose a reason for hiding this comment

vectro Mar 13, 2023

Choose a reason for hiding this comment

theory Mar 19, 2023

Choose a reason for hiding this comment

theory Mar 19, 2023

Choose a reason for hiding this comment

theory left a comment

Choose a reason for hiding this comment

vectro commented Mar 24, 2023

theory commented Mar 24, 2023

vectro commented Mar 27, 2023

theory commented Mar 27, 2023

vectro commented Mar 28, 2023

vectro commented Mar 28, 2023

vectro commented Mar 28, 2023

theory commented Apr 1, 2023

theory commented Apr 1, 2023

theory commented Apr 3, 2023

theory commented Apr 10, 2023