Add a sqitch check command that prints changes in plan for which script has changed #477

matthieu-foucault · 2019-06-10T23:29:49Z

I'm thinking about adding a --redeploy option that would revert to the appropriate change and deploy the plan again, so that the database has deployed changes matching the working directoy.

It's my first time writing in Perl, so let me know if I am missing some language idioms, or if there are some lines in there that look horribly wrong.

It's still a draft. I haven't written any test yet, and I am sure I am missing a lot of edge cases.
Happy to receive feedback either way :)

will be more efficient when sqitchers/sqitch#477 lands

theory · 2019-06-11T01:18:10Z

At a glance, it looks pretty complete, thanks! It will likely be a week or so before I can look at it in detail due to a super busy work week.

I'm thinking about adding a --redeploy option that would revert to the appropriate change and deploy the plan again, so that the database has deployed changes matching the working directoy.

Sounds like the rebase command. Or do I misunderstand? Oh, maybe you mean so it knows what version to revert to? Might be useful to add some sort of --revised option to the rebase command, I think.

matthieu-foucault · 2019-06-11T16:00:56Z

It's the rebase command indeed. I can add a --revised option that finds the parent of the first script that is different in the working directory and rebases onto that

will be more efficient when sqitchers/sqitch#477 lands

theory

This looks like a great start, thank you! I have most of my feedback in comments on this PR; I don't think it'll take you long to tweak and refactor; if you're new to Perl I can't tell!

One thing more it needs, however, is tests. Once you move the actual checks to the Engine module, I suggest adding them to t/engine.t, following the examples there for testing the verify method to test the new check method. Then add tests for the command module itself to a new test file t/check.t, and borrow from t/verify.t to get some nice test coverage.

Is that work you'd be able to do, @matthieu-foucault? This is awesome, and I'm excited to such a great new contribution.

Thank you!

lib/App/Sqitch/Command/check.pm

lib/App/Sqitch/Role/DBIEngine.pm

lib/sqitch-check.pod

t/lib/DBIEngineTest.pm

matthieu-foucault · 2019-06-18T22:10:27Z

Thanks for the review @theory ! I'll have time to work on it this week, and add the rebase --revised option mentioned above.

matthieu-foucault · 2019-06-20T00:10:34Z

Both check and rebase --revised work at this point. I should be able to work on adding tests tomorrow.

matthieu-foucault · 2019-06-21T00:23:55Z

@theory There are still tests running on TavisCI, but I think this is good for a review 🎉

matthieu-foucault · 2019-06-21T16:23:04Z

It seems that the windows build can't find Test::MockObject::Extends, which I added to be able to mock a script_hash change. @theory any idea on how to fix that?

theory · 2019-07-10T12:40:51Z

Thanks for that update. Reviewing this PR is on my list, but not sure when I will get to it. Life pretty bananas right now…appreciate your patience.

theory

Apologies for the delay in review, Matthieu. This looks great! Just a few minor tweaks and I think it's ready to go. Amazing work, really appreciate it!

theory · 2019-10-02T14:45:56Z

lib/App/Sqitch/Command/rebase.pm

 sub options {
    return qw(
        onto-change|onto=s
        upto-change|upto=s
+        revised!


I don't think we want this to be negate-able; the ! means that there is also a --no-revised option, which doesn't seem necessary. Am I missing something?

theory · 2019-10-02T14:48:18Z

lib/App/Sqitch/Engine.pm

+    return -1;
+}
+
+sub planned_deployed_common_ancestor_id {


I didn't see a test for this method, or for _find_planned_deployed_divergence_idx. Are they implicit in the tests for check?

theory · 2019-10-02T14:53:27Z

lib/sqitch-rebase.pod

@@ -63,6 +65,13 @@ L<sqitchchanges> for the various ways in which changes can be specified.
 Specify the deployment change. Defaults to the last point in the plan. See
 L<sqitchchanges> for the various ways in which changes can be specified.

+=item C<--revised>
+
+Finds the change to revert onto based on the output of L<sqitch-check>. The


Not sure this is accurate: based on the output of L<sqitch-check>. Is there output from sqitch check when you run this? I thought it was simply that it would rebase on the last change that does not conflict between the plan and the deployed changes. I think you can omit the part about the output of sqitch check from this description.

theory · 2019-10-02T14:56:55Z

lib/sqitch-check.pod

+
+=head1 Description
+
+Runs various checks on the state of the working directory and the database.


Is it "various checks" at this point? Seems like there's just the one at the moment; perhaps mention that more may be added in the future?

theory · 2020-05-02T15:57:42Z

Hey @matthieu-foucault, would you give me permission to push to your branch? Details here. I've rebased it and made a few tweaks. If I can push to your branch, it will keep this PR in-tact, which is nice for history, and I can get it merged post-haste. Thanks!

matthieu-foucault · 2020-05-02T16:20:20Z

Hi @theory , thanks for taking over that PR.
Unfortunately, I cannot allow edits from maintainers as the PR is coming from an org repo, not a my personal user repo (see https://github.community/t5/How-to-use-Git-and-GitHub/How-can-we-enable-allow-edits-from-maintainers-by-default/m-p/51971/highlight/true#M11129)

theory · 2020-05-02T17:14:06Z

Bummer. Guess I'll have to close this PR and make a new one. :-(

Closes #477 and #512.

matthieu-foucault added 3 commits June 10, 2019 15:46

Add script_hash to list of columns fetched by DBIEngine

eae429d

sqitch check compares workdir and db deploy scripts SHA1s

cca2e63

Remove Dumper use directive

087f95e

wenzowski added a commit to bcgov/cas-ggircs that referenced this pull request Jun 10, 2019

wip: automatically revert/deploy via sqitch

67864da

will be more efficient when sqitchers/sqitch#477 lands

wenzowski mentioned this pull request Jun 10, 2019

automatically revert/deploy via sqitch bcgov/cas-ggircs#93

Merged

wenzowski added a commit to bcgov/cas-ggircs that referenced this pull request Jun 12, 2019

automatically revert/deploy via sqitch

5d5c3bd

will be more efficient when sqitchers/sqitch#477 lands

theory self-requested a review June 18, 2019 21:02

theory self-assigned this Jun 18, 2019

theory requested changes Jun 18, 2019

View reviewed changes

theory added this to the v1.1.0 milestone Jun 18, 2019

theory added the feature label Jun 18, 2019

matthieu-foucault added 3 commits June 19, 2019 15:04

check command code mimics verify and added check in Engine

d7d020e

Authorship comment

61fbbc1

Add --revised option to sqitch rebase

35fbb98

matthieu-foucault added 4 commits June 20, 2019 14:27

Test the check method in Engine

304ad62

Add check command test

15c8519

Engine check handles from and to parameters

caae8e5

Add docs for rebase --revised option

6cd2a59

matthieu-foucault marked this pull request as ready for review June 21, 2019 00:03

matthieu-foucault added 2 commits June 20, 2019 17:03

Test that the --revised option gets the common ancestor id

d75f6db

Add missing script_hash in GROUP BY clause

459d745

matthieu-foucault added 2 commits July 9, 2019 15:45

Patch travis config to install Test::MockObject::Extends on windows

dce2946

Install Test::MockObject::Extends, not just its dependencies

9dfa9ba

Fix handling of from_idx in planned_deployed_common_ancestor_id

7f5f159

theory requested changes Oct 2, 2019

View reviewed changes

theory mentioned this pull request May 2, 2020

Check command #512

Merged

theory closed this May 2, 2020

theory added a commit that referenced this pull request May 2, 2020

Merge branch 'check-command' into develop

920b628

Closes #477 and #512.

theory mentioned this pull request May 9, 2020

Add command to check list deployed changes for which the deploy script has changed #475

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a sqitch check command that prints changes in plan for which script has changed #477

Add a sqitch check command that prints changes in plan for which script has changed #477

matthieu-foucault commented Jun 10, 2019

theory commented Jun 11, 2019

matthieu-foucault commented Jun 11, 2019

theory left a comment

matthieu-foucault commented Jun 18, 2019 •

edited

matthieu-foucault commented Jun 20, 2019

matthieu-foucault commented Jun 21, 2019

matthieu-foucault commented Jun 21, 2019

theory commented Jul 10, 2019

theory left a comment

theory Oct 2, 2019

theory Oct 2, 2019

theory Oct 2, 2019

theory Oct 2, 2019

theory commented May 2, 2020

matthieu-foucault commented May 2, 2020

theory commented May 2, 2020


		=head1 Description

		Runs various checks on the state of the working directory and the database.

Add a sqitch check command that prints changes in plan for which script has changed #477

Add a sqitch check command that prints changes in plan for which script has changed #477

Conversation

matthieu-foucault commented Jun 10, 2019

theory commented Jun 11, 2019

matthieu-foucault commented Jun 11, 2019

theory left a comment

Choose a reason for hiding this comment

matthieu-foucault commented Jun 18, 2019 • edited

matthieu-foucault commented Jun 20, 2019

matthieu-foucault commented Jun 21, 2019

matthieu-foucault commented Jun 21, 2019

theory commented Jul 10, 2019

theory left a comment

Choose a reason for hiding this comment

theory Oct 2, 2019

Choose a reason for hiding this comment

theory Oct 2, 2019

Choose a reason for hiding this comment

theory Oct 2, 2019

Choose a reason for hiding this comment

theory Oct 2, 2019

Choose a reason for hiding this comment

theory commented May 2, 2020

matthieu-foucault commented May 2, 2020

theory commented May 2, 2020

matthieu-foucault commented Jun 18, 2019 •

edited