Support shared configuration between migrate groups

Time to discuss our options. I have a patch for migrate_drupal at #2404401: Use dependency injection for the database connection which takes advantage of #2404397: Support injection of database configuration in SqlBase to store database credentials in the active configuration for each migration - it would of course be much cleaner to store them in one place, the group, not least because it would be much simpler to update the credentials (moving the source db to a different server, belatedly deciding to use a proper read-only SQL user instead of root, etc.).

So, I see migration_group as a new configuration entity. It would support id (machine name) and label, and apart from that could store any configuration relevant to the particular application (db credentials for any SQL-based migrations, location of files for a migration from CSV or XML, etc.). I think this should be relatively straightforward.

The main question in my mind is where to maintain the relationship between migrations and their groups. I believe we are agreed there is value is allowing a migration to appear in multiple groups. Do we extend the current migration_groups support (using machine names instead of labels)

id: d6_book
label: Drupal 6 books
migration_groups:
  - drupal_6
  - drupal_7
...

or list the migrations in the group (note the source information would be written to active configuration based on dynamically provided configuration, not provided in the original .yml file) - much like a manifest that's part of the active configuration?

id: drupal_6
label: Drupal 6
migrations:
  - d6_action_settings
  - d6_aggregator_feed
...
arguments:
  source:
    key: migrate6
    target: default
    database:
      database: my_d6_site
      username: readonly
      password: foobar
      prefix: ''
      port: '3306'
      host: 12.34.56.78
      namespace: Drupal\Core\Database\Driver\mysql
      driver: mysql

Edit: wrapped added configuration under 'arguments'

Thinking about some use cases - migrate_upgrade loading the migrations to run, a dashboard UI or drush migrate-status loading migrations to display - it seems clearly more straight-forward to me to have the group contain the list of migrations, rather than scanning all active migrations to find a matching group. The first approach makes it simpler to see what groups a migration belongs to, but that should be a relatively rare need, not to mention since there will be far fewer groups than migrations there's less to scan for the answer.

Unless anyone sees a problem with the general configuration entity approach, I'll start working on a patch.

I'm digging in, and have a hackish version of this sort of almost working with migrate_upgrade. One element I didn't account for above, however, is exactly when and how the group arguments get merged into the migration instance. If a migration is in more than one group, which one does it pull arguments from when loaded? My hack at the moment is to set migration_groups on each migration (in the active configuration) to the specific migration_group being used, then when loading the migration load the specified group and array_merge the group's arguments into the migration's configuration (not saving to active config, just using it locally). It's still not quite working right though.

I'm not seeing a clean way to deal with this at the moment (maybe just because it's Friday afternoon of a long week). Now I'm kind of feeling like addressing #2202475: Registering/instantiating migrations first - this part will make more sense if we're dealing with concrete instances. Guess I'll let it percolate over the weekend...

Finally got around to doing this. The attached patch adds MigrationGroupInterface/MigrationGroup. Group config contains a shared_configuration key, which provides default values to be applied to all migrations which reference the group through the active_groups key. When a Migration is constructed, if it contains an active_group key which is the machine name of a MigrationGroup instance, it will merge in any value in shared_configuration that is not already explicitly set. There are a couple of POC patches using this:

1. migrate_upgrade: When submitting the upgrade form, a migration group is created containing the provided database credentials, and each specific migration references is through active_group. Once the migration is run, the migrate_plus drush commands can be used to review the results.
2. migrate_plus: The drush migrate-status command will display migrations by group. Both migrate-status and migrate-import take --group arguments to filter their operations. The beer example in migrate_example creates a group and assigns the migrations to it.

Ideally, I'd like the key pointing a specific migration instance to its group to be "migration_group" rather than "active_group". Right now, we have the "migration_groups" key, but that is being used more as tagging rather than full-fledged groups. Actually, in the current migrate_drupal usage, I'd rather see a migrate_drupal-specific key "drupal_versions", since what it's really doing is listing what Drupal versions the given migration configuration applies to. I tried implementing this but got stuck trying to figure out how to define and apply the configuration schema properly, so figured I should first focus on the basic group functionality.

Why migration_group a config entity?
It's more like content that describes a migration process with migrations ER field pointing to migration config entities.

I don't see how the migration group would be content? Consider a custom migration project - you work on it in a development environment, tweaking the individual migrations' configuration (field mappings etc.) and the common configuration all migrations should have access to, and ultimately want to deploy that configuration to staging and ultimately production environments.

A migration group should have a module field to store the providing module and this should be used for dependency calculation. If migrations are config, then migration groups feel like config to me - especially since they are going to be storing configurable data.

A migration group should have a module field to store the providing module and this should be used for dependency calculation. If migrations are config, then migration groups feel like config to me - especially since they are going to be storing configurable data.

Took a stab at adding the module field and accounting for it in the dependencies, but it doesn't seem to be working - when creating a group in migrate_upgrade I added module: migrate_drupal, but uninstalling migrate_drupal did not remove the group. What am I missing?

Also, deleting a group should delete the migrations attached to it - added support for that (untested, since the group isn't getting deleted...).

Oops, accidentally committed some dpm() calls.

Note that calculateDependencies() is never called.

@mikeryan how did you create the migration group?

I had migrate_upgrade doing this:

    $group_id = 'upgrade_' . $drupal_version;
    $group_config = \Drupal::configFactory()
      ->getEditable('migrate.migration_group.' . $group_id);
    $group_config->set('id', $group_id);
    $group_config->set('label', $this->t('Upgrade from Drupal !version',
      array('!version' => $drupal_version)));
    // Make the group dependent on migrate_drupal, so it is uninstalled when
    // migrate_drupal is.
    $group_config->set('module', 'migrate_drupal');
    $group_config->set('shared_configuration.source.key', 'upgrade');
    $group_config->set('shared_configuration.source.database', $database);
    $group_config->save();

Looking in the config table, migrate.migration_group.upgrade_6 looked good.

@mikeryan you need to create an entity to use the config entity system. So something like:

$entity = entity_create('migration_group', $values);
$entity->save();

OK, so I had a false equivalence in my head between a configuration entity and the configuration that defines it. If we have a YAML file in config/install describing a configuration entity, that results in the creation of an entity, but that doesn't mean the entity and the configuration are the same thing. So creating the entity in PHP saves the configuration to the active configuration, but simply saving the configuration does not create an entity.

Basically, replacing the configFactory call with an entity_create did the job. When I uninstalled migrate_drupal, the group and all the migrations it contained were deleted. So, this patch seems to be on the right track - next step is adding some tests.

Thanks!

OK, fully ready for review. No interdiff, it would be about the same size as the full patch. These are the changes:

1. I had previously been merging group configuration in the migration constructor. It makes more sense to do it at load time.
2. Added tests, for the configuration merge and for deletion.
3. I've changed the configuration key referencing the group to 'migration_group'. The existing 'migration_groups', which is not directly related to groups in this sense, needs to be renamed, I'll add a separate issue for that.

Why do groups need weight?

Primarily because of copypasta from the Migration definition... Actually, it would be nice for UIs to provide the ability to order groups for display, but given probably 90-95% of sites using migrate will have one migration group, and 99% of the remainder no more than three or four, sorting them isn't a priority, we can hold off till someone eventually opens a minor feature request.

Missing docs

Will add, updated patch coming, thanks.

@benjy: I'm not quite sure I follow - what would be injected, the migration_group storage manager?

Thanks.

1. +++ b/core/modules/migrate/src/MigrationStorage.php
   @@ -90,4 +141,42 @@ protected function addDependency(array &$graph, $id, $dependency, $dynamic_ids)
   +              // Otherwise, the existing migration value overrides the group value.
   

   80 chars

2. +++ b/core/modules/migrate/src/Tests/MigrationGroupTest.php
   @@ -0,0 +1,113 @@
   +      'migration_groups' => array('Drupal 6'), // In migration, so will be overrideen.
   

   s/overrideen/overridden (I *think* 80 chars doesn't apply for inline comments)

Also, shouldn't we replace entity_create() by \Drupal::entityManager()->getStorage($entity_type)->create($values); ?

@benjy: Thanks for the injection!

@anavarre: Here you go.

Restoring status after testbot glitch.

+++ b/core/modules/migrate/src/Tests/MigrationGroupTest.php
@@ -0,0 +1,113 @@
+    $this->strictConfigSchema = FALSE;

Why is this needed?

This looks good - it'll be great to get this one committed.

thanks,
-mike

    migration_groups:
      type: sequence
      label: 'Migration Groups'
      sequence:
        type: string
        label: 'Group'
    migration_group:
      type: string
      label: 'Active group'

This is going to be super confusing. Also I don't get how we're going to fix the dependency problem if the active group is changeable - what is the situation where a migration can belong to more than one group?

re: confusing - see the followup issue #2466797: Rename migration_groups key to avoid confusion with MigrationGroup support.

A migration can't belong to more than one group (in the concrete sense of group we're using in this issue). A migration can have more than one tag (what migration_groups has been used for).

Can we get this back to RTBC?

Thanks.

Hrrm, let me see if I need to reroll...

The new CCK plugin manager injection conflicted with the migration group storage injection we're adding here - rerolled.

Assigning to @alexpott for his followup per his request.

In a recent meeting we agreed that migration groups should be explored in contrib as part of the migrate UI. Postponing this issue as a result.