Skip to and up to options feature #137

pedrofs · 2015-10-10T03:40:41Z

Finally now it seems right!!

Sorry for the mess.

Recommenting:
The way it was implemented it should be used like:
bin/cake migrations mark_migrated VERSION --skip-to will mark all migrations from beginning until VERSION

bin/cake migrations mark_migrated VERSION --up-to will mark all migrations from beginning to VERSION (including it)

@HavokInspiration does it seem good?

Right now there are some duplicated logic (the migrations are being marked as migrated inside execute($input, $output) and markVersionsAsMigrated($path, $versions)). I can refactor it if you intend to merge :)

HavokInspiration · 2015-10-10T11:06:58Z

src/Command/MarkMigrated.php

+     */
+    protected function isUpTo(InputInterface $input)
+    {
+        return NULL !== $input->getOption('up-to');


We usually do it the other way around with the null lowercased :

if ($var === null)

HavokInspiration · 2015-10-10T11:10:50Z

Other than my comments, this is looking good 👍

HavokInspiration · 2015-10-10T11:41:31Z

And the doc is going to need an update as well.

ravage84 · 2015-10-10T11:56:22Z

README.md

@@ -63,6 +63,12 @@ bin/cake migrations mark_migrated 20150417223600
 # Since Migrations 1.3.1, a new `all` special value for the version argumentwas added.
 # The following will mark all migrations found as migrated.
 bin/cake migrations mark_migrated all
+
+# The following option up-to will try to mark the migrations from beginning to the given version (including it)


--up-to with backticks

dereuromark · 2015-10-11T12:33:28Z

src/Command/MarkMigrated.php

+     */
+    protected function isAllVersion($version)
+    {
+        return $version == 'all' || $version == '*';


pedrofs · 2015-10-11T17:04:22Z

@HavokInspiration now I implemented it as you ask. Using --target and --exclude.

Take a look and let me know if you have any else advice :)

Cheers!

HavokInspiration · 2015-10-12T07:19:06Z

Would you mind squashing your commits into one ?

I'll try to fully review it tonight if I get the time.
Looks good from an overall point of view though.

dakota · 2015-10-12T07:34:23Z

README.md

+bin/cake migrations mark_migrated 20150417223600 --target
+
+# You can use the option `--exclude` along with `--target` and it will try to mark the migrations from beginning until the given version (excluding it)
+bin/cake migrations mark_migrated 20150417223600 --target --exclude


These options don't make sense in context of how Phinx works.

Should rather be:
bin/cake migrations mark_migrated instead of bin/cake migrations mark_migrated all
bin/cake migrations mark_migrated --target=20150417223600 instead of bin/cake migrations mark_migrated 20150417223600 --target
bin/cake migrations mark_migrated --target=20150417223600 --exclude instead of bin/cake migrations mark_migrated 20150417223600 --target --exclude

With Phinx, the target (when migrating) is always assumed to be the latest version unless a --target is specified

@dakota That is a good point.
However, when the mark_migrated command was designed, it was to only mark a bake snapshot as migrated, which is why a mandatory "target" was needed as console argument.
But now that it is becoming a full featured command, I agree it should comply with the rest of the commands.
If we are going down that path, that will break compability with userland code using the command.
It also should be known that when a snapshot is baked, all migrations up to that new one will be marked as migrated, unless we provide a way to target only one migration (even though off the top of my head, this behavior should not cause trouble).

The current way of doing it would need to still function as is (but possibly show an deprecation message).

As for marking only a single migration as migrated, perhaps a --only option is needed?

pedrofs · 2015-10-13T17:38:10Z

@HavokInspiration take a look. After your review I can go on with the squashing :)

HavokInspiration · 2015-10-14T12:04:37Z

src/Command/MarkMigrated.php

-                InputArgument::REQUIRED,
-                'What is the version of the migration? Use the special value `all` to mark all migrations as migrated.'
+                InputArgument::OPTIONAL,
+                'DEPRECATED: use `bin/cake migrations mark_migrated`'


It should be DEPRECATED: use bin/cake migrations mark_migrated --target=VERSION instead

HavokInspiration · 2015-10-14T12:08:19Z

src/Command/MarkMigrated.php

+    }
+
+    /**
+     * Checks if the it is for only one migration


This comment should be corrected.

HavokInspiration · 2015-10-14T12:24:16Z

Looks good to me once corrected. My comments are mainly around docblocks and output.

pedrofs · 2015-10-15T13:34:14Z

@HavokInspiration I did the squashing using git rebase -i origin/master and git push --force and now there are two commits that shouldn't be there (they already are on master).

Before starting out this branch I updated my master branch getting changes from upstream. I think thats because there are these two commits.

Do you know how can I fix it?

Thanks.

HavokInspiration · 2015-10-15T13:55:24Z

You need to rebase your branch on cakephp repo master branch.
Add the cakephp repo as upstream, merge upstream/master in your local master and rebase on it.

Something like that :

git remote add upstream https://github.com/cakephp/migrations.git
git checkout master
git fetch upstream
git merge upstream/master
git checkout skip_to_and_up_to_options
git rebase master

This will make the diff of your branch start from the cakephp repo master branch.
And then do the git push with --force

pedrofs · 2015-10-15T14:06:07Z

@HavokInspiration thanks man. It seems good now!

ps: I used git rebase upstream/master instead

Upgrade the `mark_migrated` to match other phinx command behavior

HavokInspiration · 2015-10-16T11:47:22Z

Yes, I do that because I usually sync the master of my fork. But that works too !

👍 great work @pedrofs

Follow up to #137

HavokInspiration reviewed Oct 10, 2015
View reviewed changes

HavokInspiration added this to the 1.4 milestone Oct 10, 2015

HavokInspiration added the enhancement label Oct 10, 2015

HavokInspiration mentioned this pull request Oct 10, 2015

RFC: mark_migrated "up to" a certain version number #129

Closed

ravage84 reviewed Oct 10, 2015
View reviewed changes

HavokInspiration self-assigned this Oct 11, 2015

dereuromark reviewed Oct 11, 2015
View reviewed changes

dakota reviewed Oct 12, 2015
View reviewed changes

HavokInspiration reviewed Oct 14, 2015
View reviewed changes

src/Command/MarkMigrated.php

}

/**

* Checks if the it is for only one migration

Copy link

Member

HavokInspiration Oct 14, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment should be corrected.

pedrofs force-pushed the skip_to_and_up_to_options branch 2 times, most recently from 3b36e31 to de3bdff Compare October 15, 2015 13:30

turning mark_migrated into a full-feature command

44742c1

pedrofs force-pushed the skip_to_and_up_to_options branch from de3bdff to 44742c1 Compare October 15, 2015 14:04

HavokInspiration added a commit that referenced this pull request Oct 16, 2015

Merge pull request #137 from pedrofs/skip_to_and_up_to_options

f5bdfb5

Upgrade the `mark_migrated` to match other phinx command behavior

HavokInspiration merged commit f5bdfb5 into cakephp:master Oct 16, 2015

HavokInspiration mentioned this pull request Oct 16, 2015

Follow up to #137 #142

Merged

lorenzo added a commit that referenced this pull request Oct 16, 2015

Merge pull request #142 from cakephp/cleanup-input-option

8fd5d9f

Follow up to #137

HavokInspiration mentioned this pull request Oct 16, 2015

Update the migrations plugin book with the recent changes cakephp/docs#3406

Merged

This was referenced Jan 11, 2016

Update the markMigrated method of the Migrations class #183

Closed

Migrations markMigrated() update (non-shell) #185

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip to and up to options feature #137

Skip to and up to options feature #137

pedrofs commented Oct 10, 2015

HavokInspiration Oct 10, 2015

HavokInspiration commented Oct 10, 2015

HavokInspiration commented Oct 10, 2015

ravage84 Oct 10, 2015

dereuromark Oct 11, 2015

pedrofs commented Oct 11, 2015

HavokInspiration commented Oct 12, 2015

dakota Oct 12, 2015

HavokInspiration Oct 12, 2015

dakota Oct 12, 2015

pedrofs commented Oct 13, 2015

HavokInspiration Oct 14, 2015

HavokInspiration Oct 14, 2015

HavokInspiration commented Oct 14, 2015

pedrofs commented Oct 15, 2015

HavokInspiration commented Oct 15, 2015

pedrofs commented Oct 15, 2015

HavokInspiration commented Oct 16, 2015

Skip to and up to options feature #137

Skip to and up to options feature #137

Conversation

pedrofs commented Oct 10, 2015

Choose a reason for hiding this comment

HavokInspiration commented Oct 10, 2015

HavokInspiration commented Oct 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrofs commented Oct 11, 2015

HavokInspiration commented Oct 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrofs commented Oct 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HavokInspiration commented Oct 14, 2015

pedrofs commented Oct 15, 2015

HavokInspiration commented Oct 15, 2015

pedrofs commented Oct 15, 2015

HavokInspiration commented Oct 16, 2015