Splitting this off from #850142: Integrate VCAPI with project_release node add form...

Right now, cvs.module has a pretty hacky way to allow sites to define their own mapping from CVS branch and tag names into project_release versions. This isn't just about the version string itself, but really about how the elements of the branch or tag name map into the component elements of the release node's version object (e.g. major, minor, patch, extra, etc).

myself, mikey_p, and sdboyer hashed this out in IRC for a little while tonight, and decided that it makes the most sense to just keep using ctools plugins for this, since VCAPI already has a fair bit of ctools plugins for various things.

That also means we're going to want to define which vc_release version mapper plugin to use for each repository in the $repo->plugins array. That way, if a site needs to override our default plugins, they can just put something like this in a site-specific module:

  $repo->plugins['vc_release_version_mapper'] = 'some_plugin';
  $repo->save();

I'll be working on this over the next couple of days, since it's one of the major blockers needed to finish off #850142...

CommentFileSizeAuthor
#17 1015296-16A.VersioncontrolReleaseLabelVersionMapperGeneric.patch1.96 KBdww
#17 1015296-16B.site-wide-plugin-default.patch975 bytesdww
#15 1015296-15.vc_release_label_version_mapper_docs_fixes.patch2.18 KBdww
#13 1015296-13.vc_release_label_version_mapper_docs_fixes.patch3.43 KBdww
#11 1015296-11.vc_release_label_name_version_mapper.patch4.9 KBdww
#8 1015296-8.vc_release_label_version_mapper_followup.patch3.98 KBdww
#7 1015296-7.drupalorg_git_mapper.patch5.29 KBdww
#5 1015296-5.vc_release_label_version_mapper_ctools_plugin.patch14.35 KBdww
#3 1015296-3.vc_release_label_version_mapper_ctools_plugin.patch14.5 KBdww
#1 1015296-1.vc_release_label_version_mapper_ctools_plugin.patch12.55 KBdww

Comments

dww’s picture

dww
we/he/they
commented
Status: Active » Needs review
StatusFileSize
new 1015296-1.vc_release_label_version_mapper_ctools_plugin.patch12.55 KB

Completely untested, but here's a pretty good start. I didn't look at the generic mapping code at all -- that's just the original logic from jpetso moved into a "generic" ctools plugin. I *think* I got all the ctools plumbing right. It'd be great to get some eyes on this for the basic approach. I'll try to actually test this in the meantime. ;) If it doesn't horribly explode, I'll start writing a plugin for the d.o Git mapping we'll need.

mikey_p’s picture

mikey_p commented
Status: Needs review » Needs work

Discussed this in IRC with dww: moving the logic for returning the object for the correct plugin into a separate helper function. Apparently this is also missing the from_branch() function.

dww’s picture

dww
we/he/they
commented
Status: Needs work » Needs review
StatusFileSize
new 1015296-3.vc_release_label_version_mapper_ctools_plugin.patch14.5 KB

Indeed. Try this.

sdboyer’s picture

sdboyer commented

Real quick, re: plugin loading:

    if (!empty($repo->plugins['versioncontrol_release_label_version_mapper'])) {
      $plugins = ctools_get_plugins('versioncontrol_release', 'label_version_mapper');
      foreach ($plugins as $plugin_name => $plugin_definition) {
        if ($plugin_name == $repo->plugins['versioncontrol_release_label_version_mapper']) {
          $class = ctools_plugin_get_class($plugin_definition, 'handler');
          $mapper = new $class();
          return $mapper->GetVersionFromTag($tag, $project_node);
        }
      }
    }

Can be really simplified:

    if (!empty($repo->plugins['versioncontrol_release_label_version_mapper'])) {
      if ($class = ctools_plugin_load_class('versioncontrol_release', 'label_version_mapper', $repo->plugins['versioncontrol_release_label_version_mapper'], 'handler')) {
        $mapper = new $class();
        $mapper->GetVersionFromTag($tag, $project_node);
      }
    }

Kind of a long line there, but yeah.

dww’s picture

dww
we/he/they
commented
StatusFileSize
new 1015296-5.vc_release_label_version_mapper_ctools_plugin.patch14.35 KB

Cool, I figured there must have been a better way. ;) Don't we need the last argument to be 'mapper' given how generic.inc is setup? Like so?

dww’s picture

dww
we/he/they
commented
Status: Needs review » Active

Okay, committed #5 to HEAD: http://drupal.org/cvs?commit=476264

Now we need the d.o-specific Git mapping we want as a separate plugin...

dww’s picture

dww
we/he/they
commented
Priority: Major » Critical
Status: Active » Needs review
StatusFileSize
new 1015296-7.drupalorg_git_mapper.patch5.29 KB

Here's a first-stab at the d.o-specific mapping. I tested the regexps in here pretty carefully, but I haven't actually tried running the plugin itself. Anyone have any objections or see any problems? Filenames sane? Should this live in vc_release or in drupalorg_project?

Also, since this is blocking #850142: Integrate VCAPI with project_release node add form and essential to being able to launch the Git migration, bumping to critical...

dww’s picture

dww
we/he/they
commented
StatusFileSize
new 1015296-8.vc_release_label_version_mapper_followup.patch3.98 KB

Given my latest patch in #1019162: port vc_release to VCAPI V2 I'm actually testing this now. Here's a follow-up patch to clean up some stuff. Still need to decide where to commit the drupalorg_git plugin from #7...

dww’s picture

dww
we/he/they
commented

Also, now that I'm really testing this and how suitable it is for #850142: Integrate VCAPI with project_release node add form, I've found one design flaw in the plugin as it currently stands.

During various steps in the multi-page release node form, we currently only save the label_id to $form_state. The plugin currently expects an entire Versioncontrol(Branch|Tag) object. So, we have 3 options:

A) Keep re-loading the full object during various steps in the form alter given the label_id so we can hand it off to the plugin. we'd need to stash $label_type in $form_state in addition to $label_id, so we know which load(Tags|Branches) method to call on our project's $repo object. Otherwise this would work, although it seems pretty heavy-weight to load the whole object given that all we actually care about is the label name.

B) Stash the full branch|tag object in $form_state. Again, since the object is huge (at least according to devel), this seems like overkill.

C) Change the plugin to just take the label name. This would also allow us to have a unified GetVersionFromLabel($name, $type) interface, instead of forcing separate methods since there's no base class shared between Versioncontrol(Branch|Tag). Then, all we have to stash in $form_state is the $label_id, $label_name and $label_type. Actually, we don't even really need $label_id at all, although I guess it doesn't hurt since it's a single int. I can't fathom why a plugin would need the full object. And, if it did, it already gets the $project node, so worst case, it could load it itself if it really needed (since a label name should be unique within a given project).

I'm leaning towards C. mikey_p is, too. Any thoughts/objections?

dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work

sdboyer agrees with #9.C, too. So that's what I'm going to do. That needs to happen before #7 can be committed. And I think we're all in agreement that should live in drupalorg_versioncontrol in the new DRUPAL-6--3 branch of http://drupal.org/project/drupalorg...

dww’s picture

dww
we/he/they
commented
Status: Needs work » Needs review
StatusFileSize
new 1015296-11.vc_release_label_name_version_mapper.patch4.9 KB

Here's #9.C for the interface and the generic plugin.

dww’s picture

dww
we/he/they
commented
Status: Needs review » Fixed

#7 was moved to #1019244: Add a versioncontrol_release label_version_mapper plugin for d.o-specific mapping of Git branches and tags to d.o releases (and is now committed to DRUPAL-6--3 of drupalorg_versioncontrol).

I committed both #8 and #11 to HEAD of vc_release.

Calling this done, and moving on to #1019162: port vc_release to VCAPI V2...

dww’s picture

dww
we/he/they
commented
StatusFileSize
new 1015296-13.vc_release_label_version_mapper_docs_fixes.patch3.43 KB

Whoops, just noticed some docs are still stale after #11. Also, while we're at it, might as well add a versioncontrol_release_get_version_from_label() method, too.

dww’s picture

dww
we/he/they
commented

Committed #13 to HEAD.

dww’s picture

dww
we/he/they
commented
StatusFileSize
new 1015296-15.vc_release_label_version_mapper_docs_fixes.patch2.18 KB

Noticed a few more doxygen bugs. Committed the attached.

dww’s picture

dww
we/he/they
commented
Status: Fixed » Needs work

Two bugs still in here:

A) VersioncontrolReleaseLabelVersionMapperGeneric is broken due to some public vs. protected mismatch, and not using $this-> in a few places.

B) It's sorta dumb to require that every single repo object has its own plugin defined for this. Generally, this is just going to be a site-wide thing. So, we should have a site-wide default variable_get() that we inspect before we fallback to the generic.inc plugin.

Working on it now, since this is in the way of being able to properly test/demo release nodes on git-dev.d.o

dww’s picture

dww
we/he/they
commented
Status: Needs work » Fixed
StatusFileSize
new 1015296-16B.site-wide-plugin-default.patch975 bytes
new 1015296-16A.VersioncontrolReleaseLabelVersionMapperGeneric.patch1.96 KB

Committed the attached two patches to vc_release HEAD.

Status: Fixed » Closed (fixed)
Issue tags: -git phase 2, -git sprint 8

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +git phase 2, +git sprint 8

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.