Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Option to require verify scripts during deployment #799

Open
ewie opened this issue Oct 24, 2023 · 3 comments
Open

Option to require verify scripts during deployment #799

ewie opened this issue Oct 24, 2023 · 3 comments
Assignees

Comments

@ewie
Copy link
Contributor

ewie commented Oct 24, 2023

It would be great if sqitch deploy had an option to require a verify script for each deploy script. Right now, missing verify scripts are only logged without failing the deployment when running sqitch deploy --verify, for example:

  + foo .. Verify script verify/foo.sql does not exist
ok
  + bar .. ok

I recently had this situation when a team mate accidentally moved some of the verify scripts. The test runner did not fail but luckily I spotted the moved files during code review. A failing test run would've been better in catching this right away.

Of course I can add a step to the test runner to manually check the files. Something like:

for vf in $(find deploy -name '*.sql' | sed 's/deploy/verify/'); do
  if [ ! -f "$vf" ]; then
    echo "not found: $vf"
    exit 1
  fi
done

But I think it's useful if Sqitch can support this out of the box. Also because this script above makes assumptions about the directory layout which is quite flexible in Sqitch.

I'm thinking about a new option --require-verify:

sqitch deploy --verify --require-verify

Maybe even imply --verify when deploying with --require-verify so that it behaves as a stronger variant of --verify. I think the option name fits nicely with the existing --no-verify to allow different verification levels, i.e. --no-verify < --verify < --require-verify.

@theory theory self-assigned this Oct 26, 2023
@theory
Copy link
Collaborator

theory commented Oct 26, 2023

Yeah, optionally requiring verify scripts makes a ton of sense.

@theory
Copy link
Collaborator

theory commented Oct 29, 2023

Given that we would want to support the same feature with the same name in the verify command, I think --require-verify wouldn't quite cut it. Maybe strict?

sqitch deploy --strict-verify
sqitch verify --strict

But its semantics will vary from the existing revert strict config, which requires the specification of a version to revert to.

Sketch implementation:

  • Add strict verify (or other named) attribute to App::Sqitch::Engine
  • Modify verify_change() in App::Sqitch::Engine if the new attribute is true
  • Add appropriate options and config variables to App::Sqitch::Command::deploy and ::verify and document them in the sqitch-deploy*.pod and sqitch-revert*.pod files

@ewie
Copy link
Contributor Author

ewie commented Nov 1, 2023

Given that we would want to support the same feature with the same name in the verify command, I think --require-verify wouldn't quite cut it. Maybe strict?

Yes, --strict-verify sounds better than --require-verify. Haven't thought about sqitch verify because I don't use it.

Add appropriate options and config variables [...]

Should it be a separate config variable deploy.strict_verify? Or can we extend deploy.verify to also accept value verify in addition to values accepted by Types::Standard::Bool?

A separate config variable may be misleading:

[deploy]
verify = false
strict_verify = true

Or do we just say that deploy.strict_verify overrides deploy.verify? Same question also for when the user wants to run sqitch deploy --no-verify --strict-verify.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants