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

Add the migrate runner commands #3402

Closed
wants to merge 10 commits into from
Closed

Add the migrate runner commands #3402

wants to merge 10 commits into from

Conversation

claudiu-cristea
Copy link
Member

@claudiu-cristea claudiu-cristea commented Feb 20, 2018

Resolves #2926

Should be fixed before merging:

  • https://www.drupal.org/project/migrate_run/issues/2914429 ✔️
  • Migrate import runs dependent migrations multiple times. ✔️
  • Add documentation on how to write and run migrations. ✔️
  • Add a doc section specifically telling folks how to get started with Drupal => Drupal upgrades. How to make use of the migrations that are in core.
  • Move Copy example modules from migrate_plus into https://www.drupal.org/project/examples. Examples provide in Migrate Plus are coupled to the module so, probably we cannot move them, we should create some adapted versions in https://www.drupal.org/project/examples.
  • This plan was drafted with Lucas Hedding (@heddn) and Moshe Weitzman (@weitzman). Prepare migrate_tools, migrate_plus to adapt the new Drush core feature. We need to coordinate new releases of Migrate Plus & Migrate Tools and also add the composer constraints in Drush:
    • Drush 9 commands will be removed from migrate_tools because they are provided now in core. When used with Drush 8, the commands are still provided in the *.drush.inc file.
    • Module migrate_tools will alter the Drush core commands migrate:status, migrate:import and migrate:rollback in order to support migration grouping, which is a feature of Migrate Plus. A new branch (8.x-5.x) will hold these changes. Drupal.org issue #2952465.
  • Update https://docs.google.com/document/d/1ROgc6OIepmcIPFFCb63tDiKz1aK1pXW3seyYN7rgfLs/edit#

Other improvements can be added after merging.

Related issues

@claudiu-cristea
Copy link
Member Author

claudiu-cristea commented Feb 20, 2018

Just note here. I needed a namespace for some classes. I have no idea if my pick was correct: Drush\Drupal\Migrate.

Moshe: LGTM

@weitzman
Copy link
Member

weitzman commented Feb 24, 2018

One thing I did not consider earlier is documentation. Folks need to learn how to write and run migrations. Where should we point them to? I'm a little concerned they will use this issue queue for general migrate support.

@claudiu-cristea
Copy link
Member Author

claudiu-cristea commented Mar 4, 2018

I moved the commands from Drush\Commands\core under Drush\Drupal\Commands\core. @weitzman, could you, please, check the @todo from MigrateRunnerCommands::getMigrationPluginManager()?

@claudiu-cristea
Copy link
Member Author

This is ready for review.

@claudiu-cristea
Copy link
Member Author

This is blocked now in #3447 because we need that for the migrate_plus patch.

@weitzman
Copy link
Member

weitzman commented Mar 8, 2018

I added a todo about docs for Drupal => Drupal upgrade migrations.

@claudiu-cristea
Copy link
Member Author

Reworked the issue summary and added a first try for Migrate Plus in https://www.drupal.org/project/migrate_plus/issues/2951968

@heddn
Copy link
Contributor

heddn commented Mar 12, 2018

Add a doc section specifically telling folks how to get started with Drupal => Drupal upgrades. How to make use of the migrations that are in core.

I'm really happy with what I'm seeing going on here. I do want to be cautious about the D2D migration docs. Something like the following might be a good start.

Running drush migrate:status and other core drush migrate commands will not work out of the box for core migrations. Unless site owners have created a 'migrate' db entry in their settings(.local).php. This is because all Core the source plugins are all SQL based and require a defined connection. There is a fallback to a db key called 'migrate'. But that's kinda an evil hack and only supports a single DB at a time. If you want to support more than a single DB source, then install migrate_plus and use migrate groups instead.

^ is probably way too technical, but it is a start.

@Eli-TW
Copy link

Eli-TW commented Mar 12, 2018

Question with respect to this approach - if migrate_plus alters the core commands rather than migrate_tools, that means people wanting to use other parts of migrate_plus (eg the prepareRow event, the JSON/XML source plugins) will be forced to use its commands rather than the Drush ones, whereas before it would have been possible to use migrate run with those source plugins. Is that correct? And is that a limitation that we have decided to accept?

@heddn
Copy link
Contributor

heddn commented Mar 12, 2018

The prepare row event and the JSON/XML and CSV sources will not be effected by Drush core being the runner.

@claudiu-cristea
Copy link
Member Author

@Eli-TW this is a good question. The prepare row event is now provided also by this PR, so that will work also without migrate_plus. But the module also provides some plugins. For this reason, I think, it would be correct to consider moving the command alters in migrate_tools (?)

@heddn
Copy link
Contributor

heddn commented Mar 12, 2018

@claudiu-cristea do we want to remove the prepare row event from drush core? Or bring it in separately? That would make this focused on the drush commands only then. And we can discuss the event in a follow-up.

@claudiu-cristea
Copy link
Member Author

claudiu-cristea commented Mar 12, 2018

@heddn That event is critical to migrate runner, so we cannot remove it from Drush core. We can, probably, rename it to avoid any confusion (DRUSH_PREPARE_ROW) or, better, move it to Drupal core.

@heddn
Copy link
Contributor

heddn commented Mar 12, 2018

@claudiu-cristea idealy, moving to core would ideally happen. Not sure if it will, but I'd like it. That depends on commiter discretion.

I did a quick scan of the code here in this PR, what is using it inside of Drush? Why is MigratePrepareRowEvent critical?

@claudiu-cristea
Copy link
Member Author

@claudiu-cristea
Copy link
Member Author

claudiu-cristea commented Mar 12, 2018

There's a Drupal core issue to add the event to the migrate core module. However, the issue scope seems broader. Probably adding only the event dispatcher could be split into a separate issue https://www.drupal.org/project/drupal/issues/2488836

@heddn
Copy link
Contributor

heddn commented Mar 12, 2018

Hmm, I wonder if we could bring the support for idlist in an implementation of the core prepare_row hook for now? With a TODO to a new issue to add it more better. Because ideally we'd have the event in core.

@claudiu-cristea
Copy link
Member Author

I split the event thing in https://www.drupal.org/project/drupal/issues/2952291

@claudiu-cristea
Copy link
Member Author

claudiu-cristea commented Mar 12, 2018

@heddn using the hook could be also an idea. So we avoid all the event headache. EDIT: this is not gonna work. See below why

@heddn
Copy link
Contributor

heddn commented Mar 12, 2018

@claudiu-cristea I've re-booted the core issue. But it more than likely not going to land in 8.5, so let's proceed with a hook for the mean time?

@claudiu-cristea
Copy link
Member Author

claudiu-cristea commented Mar 12, 2018

As we've discussed in the chat and because implementing a hook doesn't work (because the migrate executable is not accessible from the hook implementation), we've decided to keep the Drush core event, even if, eventually, will be redundant with the migrate_plus prepare row event. Both will become obsolete after the event will land in Drupal core, in https://www.drupal.org/project/drupal/issues/2952291. In order to prevent any confusion, we renamed the event from PREPARE_ROW to DRUSH_MIGRATE_PREPARE_ROW.

@weitzman
Copy link
Member

I've decided not to add to confusion by merging this. migrate_run is a great project and will remain separate from Drush core for now.

@weitzman weitzman closed this May 16, 2018
@weitzman
Copy link
Member

Just want top say that I did a large migration with Migrate Run recently and worked really well. I am again happy to merge this. The source repo was deleted but the code here is recoverable via https://patch-diff.githubusercontent.com/raw/drush-ops/drush/pull/3402.patch

@heddn
Copy link
Contributor

heddn commented May 26, 2020

There's been several new features added to migrate_tool's drush runner recently that might be considered useful. I'd be interested if using it as the basis might be another option.

@heddn
Copy link
Contributor

heddn commented May 26, 2020

There's also pretty complete testing of the commands at this point too. I'd be more then happy to move those things over into drush core.

@weitzman
Copy link
Member

I'm interested in new runner features, for sure. And tests. What I'm not so interested in is migrations as config entities. Is that a built-in assumption for drush_tools?

@heddn
Copy link
Contributor

heddn commented May 26, 2020

@weitzman I think it could be optional. If we wrote it so that the list of migrations be decorated or something. That way migrate_tools could provide config entities without having to support all the runner capabilities.

@claudiu-cristea
Copy link
Member Author

The migrate_run is only using Drupal core. Nothing more. For this reason, I think, it better suits Drush.

@heddn
Copy link
Contributor

heddn commented May 26, 2020

In the area where we've agreed to differ is the fact that migrate_tools also finds migrate_plus config entities. But tools has added several new features in the past few months, namely support for syncing, progress bar, id-list, continue-on-failure. These are features it would be nice to maintain. If we have to drop config entities, I'd be inclined to do that and then figure out a way to re-add that support via some drush alter hook or something.

@claudiu-cristea
Copy link
Member Author

Then migrate_tools should implement command hooks that are adding migrations from configs to the discovery.

@heddn
Copy link
Contributor

heddn commented May 26, 2020

I'm glad we can all agree :)

claudiu-cristea added a commit to claudiu-cristea/drush that referenced this pull request Dec 15, 2020
claudiu-cristea added a commit to claudiu-cristea/drush that referenced this pull request Dec 15, 2020
claudiu-cristea added a commit to claudiu-cristea/drush that referenced this pull request Dec 18, 2020
claudiu-cristea added a commit to claudiu-cristea/drush that referenced this pull request Dec 31, 2020
weitzman pushed a commit that referenced this pull request Jan 3, 2021
* Initial commit with code from PR #3402.

* Apply https://www.drupal.org/project/migrate_run/issues/3184771.

* Fix deprecated assertions.

* Remove faulty debug statement.

* Try a fix for PHP8

* Try to fix on Win OS.

* Avoid passing user data by reference.

* Restore composer.lock

* New Windows fix.

* Resolve the query.

* Regression test for https://www.drupal.org/project/migrate_tools/issues/2919108.

* Typo

* Fix PHPUnit deprecation.

* Debug Windows tests.

* Only check migration output.

* Relax the regexp.

* Win debug.

* Fix Windows tests.

* Regression test for https://www.drupal.org/project/migrate_tools/issues/3015386

* Fix source od random failures.

* Coding standards.

* Apply https://www.drupal.org/project/migrate_tools/issues/3011996

* Fix strict typing in MigrateRunnerCommands

* Coding standards.

* Test user entered invalid migrations

* Coding standards.

* Test failures on rollback

* Revisit command arguments & options, camelize variables.

* Progress bar. See https://www.drupal.org/project/migrate_tools/issues/3075131

* Make the progress bar output predictable

* Fix Win test.

* Coding standards.

* Partially apply https://www.drupal.org/project/migrate_tools/issues/2798363:

- Not applied for migrate:message as it don't work in Migrate Tooles either. No tests.
- Not implemented --idlist-delimiter: (1) the naming is miselading, it refers to columns
  of the same ID rather than intra-ID delimiter and (2) I see no usecase (why ':' is not
  enough?)

* Improve documentation.

* Conflicts with drupal/migrate_tools and drupal/migrate_run

* Implement https://www.drupal.org/project/migrate_tools/issues/2809433

* What is the origin of this strange @validate-module-enabled tag?

* Don't add comma after last param.

* Ensure a test that works regardless of Symfony version.

* Use the <info> tag.

* Move migrate_runner.inc.

* Use ::success() instead of ::log()

* Exception instead of log error

* Use StringUtils::csvToArray()

* Simple progress bar on Windows.

* Apply https://www.drupal.org/project/migrate_tools/issues/2960221

* Another Windows fix tentative.

* Test fix follow-up

* Minor docs fix.

* Re-fix reverted fix ;)

* Reverted by mistake

* Remove @drupal-dependencies

* Use ->success() instead of ->log()

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

Successfully merging this pull request may close these issues.

Add migrate_run instead of migrate_tools
4 participants