Maven Changes Plugin
  1. Maven Changes Plugin
  2. MCHANGES-259

Create modularity for supporting multiple issue management systems

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.6
    • Component/s: other issue-trackers
    • Labels:
      None

      Description

      In discussion of MCHANGES-245, Dennis notes the need for some global modularity for capturing the behavior of the different issue management systems. I'm creating this JIRA as a hat-rack to hang this work off of.

      My plan is to start by creating something very simple in the way of an abstract class, which can be elaborated as we go.

      1. MCHANGES-259.patch
        18 kB
        Benson Margulies

        Activity

        Benson Margulies created issue -
        Benson Margulies made changes -
        Field Original Value New Value
        Assignee Benson Margulies [ bmargulies ]
        Hide
        Benson Margulies added a comment -

        Dennis,

        To save churn, I wonder if I could elicit more thought from you as to the shape of this? My first thought is an abstract class that stored an issue type map and then had some functions for the IssueAdapter to call. Does that seem reasonable?

        Show
        Benson Margulies added a comment - Dennis, To save churn, I wonder if I could elicit more thought from you as to the shape of this? My first thought is an abstract class that stored an issue type map and then had some functions for the IssueAdapter to call. Does that seem reasonable?
        Hide
        Dennis Lundberg added a comment -

        I was thinking of having an Interface (or abstract class to start with) that holds the mappings. Perhaps we can make do with a simple Map object for it. That Map object would then be sent to the IssueAdapter from an (IMS specific) implementation of a report or announcement.

        One of the problems with the current plugin design is that there is a lot of duplicate code between the Report mojo and the Announcement mojo, when it comes to parameters. The parameters often needs to be duplicated between the two mojos for every new IMS. What I would like is to have some kind of a configuration object that is specific to each IMS.

        One way to do it would be to have something similar to the <archive> configuration used in the archiving plugins (JAR, WAR, EAR). That configuration is a Modello model which is reused between several plugins. Unfortunately that would require people to change their configuration for the Changes Plugin, but on the other hand we would get something that is much easier to maintain and extend.

        Show
        Dennis Lundberg added a comment - I was thinking of having an Interface (or abstract class to start with) that holds the mappings. Perhaps we can make do with a simple Map object for it. That Map object would then be sent to the IssueAdapter from an (IMS specific) implementation of a report or announcement. One of the problems with the current plugin design is that there is a lot of duplicate code between the Report mojo and the Announcement mojo, when it comes to parameters. The parameters often needs to be duplicated between the two mojos for every new IMS. What I would like is to have some kind of a configuration object that is specific to each IMS. One way to do it would be to have something similar to the <archive> configuration used in the archiving plugins (JAR, WAR, EAR). That configuration is a Modello model which is reused between several plugins. Unfortunately that would require people to change their configuration for the Changes Plugin, but on the other hand we would get something that is much easier to maintain and extend.
        Benson Margulies made changes -
        Attachment MCHANGES-259.patch [ 55524 ]
        Hide
        Benson Margulies added a comment -

        Dennis,

        The attached patch is a lot of changes that try to start the modularity you are looking for. This is complex enough that I'd like some feedback from you before committing; I don't want to have to back it out.

        --benson

        Show
        Benson Margulies added a comment - Dennis, The attached patch is a lot of changes that try to start the modularity you are looking for. This is complex enough that I'd like some feedback from you before committing; I don't want to have to back it out. --benson
        Hide
        Benson Margulies added a comment -

        Note: this is a start. I didn't get involved in refactoring the unshared code.

        To be perfectly honest, I'm not that enthralled with this plugin at all. But I'd like to use it, and I can't without some support for customized JIRAs. I'm willing to do some work to get those changes to the point where everyone is comfy, but not to take on a giant rewrite.

        Show
        Benson Margulies added a comment - Note: this is a start . I didn't get involved in refactoring the unshared code. To be perfectly honest, I'm not that enthralled with this plugin at all. But I'd like to use it, and I can't without some support for customized JIRAs. I'm willing to do some work to get those changes to the point where everyone is comfy, but not to take on a giant rewrite.
        Hide
        Dennis Lundberg added a comment -

        Benson,

        Some thoughts and questions after a brief look at your patch.

        • Lots of good stuff in there with the abstract class and all
        • I haven't worked with enums before. What data is stored in an "instance" of IssueType?
        • Is DefaultIssueManagmentSystem really needed? Or should it be renamed to JIRAIssueManagementSystem
        • Was there a particular reason to change the logic in AnnouncementMojo? The reason I ask is that I envision changes.xml to be just another IMS in the future, and the changes to the logic seems to go against that. I.e it hard codes the changes + 1 that we currently have, making it harder to change that going forward.

        A couple of more things I'd like to do:

        • Add an interface IssueManagementSystem and use that wherever possible in place of AbstractIssueManagementSystem
        • Add a constant in IssueAdapter:
          final String UNKNOWN_TYPE = "";
        Show
        Dennis Lundberg added a comment - Benson, Some thoughts and questions after a brief look at your patch. Lots of good stuff in there with the abstract class and all I haven't worked with enums before. What data is stored in an "instance" of IssueType? Is DefaultIssueManagmentSystem really needed? Or should it be renamed to JIRAIssueManagementSystem Was there a particular reason to change the logic in AnnouncementMojo? The reason I ask is that I envision changes.xml to be just another IMS in the future, and the changes to the logic seems to go against that. I.e it hard codes the changes + 1 that we currently have, making it harder to change that going forward. A couple of more things I'd like to do: Add an interface IssueManagementSystem and use that wherever possible in place of AbstractIssueManagementSystem Add a constant in IssueAdapter: final String UNKNOWN_TYPE = "";
        Hide
        Benson Margulies added a comment -

        1. Enums. For each constant listed in the enum, there is one instance for the entire JVM. It has whatever fields and methods you declare on the enum. So, you are declaring a collection of named singletons which have a common contract.

        2. Here's the modularity dilemma as I saw it. If we want to seriously support multiple IMSs, there is a lot of work to do. The comment at the top that we only support 'changes + 1' is there for a good reason given the code. So, I felt that I either had to tackle the big job of actually making it operate on and iterate over multiple systems, or I wanted to enforce the restriction. Now, I could scope the creation and use of the new object(s) to within the blocks for the individual IMSs, I realize, and leave things no worse off then they were. I'll do that.

        3. If you prefer an interface, I think I'll rename the class from Abstract to Default, and add the interface. Unless you prefer the non-JIRA case to start out with no mappings, in which case I'd add the JIRA subclass.

        Generally, I want to point out that this whole business is somewhat off to one side of the patch that started all this. All IMSes that I know are configurable. So, with or without this stuff, we still need to keep the original patch code that allows the user to configure their own mappings. I wonder if we should release what's currently checked in before continuing with any variation on this.

        Show
        Benson Margulies added a comment - 1. Enums. For each constant listed in the enum, there is one instance for the entire JVM. It has whatever fields and methods you declare on the enum. So, you are declaring a collection of named singletons which have a common contract. 2. Here's the modularity dilemma as I saw it. If we want to seriously support multiple IMSs, there is a lot of work to do. The comment at the top that we only support 'changes + 1' is there for a good reason given the code. So, I felt that I either had to tackle the big job of actually making it operate on and iterate over multiple systems, or I wanted to enforce the restriction. Now, I could scope the creation and use of the new object(s) to within the blocks for the individual IMSs, I realize, and leave things no worse off then they were. I'll do that. 3. If you prefer an interface, I think I'll rename the class from Abstract to Default, and add the interface. Unless you prefer the non-JIRA case to start out with no mappings, in which case I'd add the JIRA subclass. Generally, I want to point out that this whole business is somewhat off to one side of the patch that started all this. All IMSes that I know are configurable. So, with or without this stuff, we still need to keep the original patch code that allows the user to configure their own mappings. I wonder if we should release what's currently checked in before continuing with any variation on this.
        Hide
        Dennis Lundberg added a comment -

        2. That'd be great.

        3. I prefer to have an interface, and an abstract class with common implementation pieces. Also a JIRA subclass is better than a general purpose Default implementation, since the implementation in there is specific to JIRA.

        The patch for this issue doesn't contradict your original patch to make the mappings configurable. IMO we should do both.

        Show
        Dennis Lundberg added a comment - 2. That'd be great. 3. I prefer to have an interface, and an abstract class with common implementation pieces. Also a JIRA subclass is better than a general purpose Default implementation, since the implementation in there is specific to JIRA. The patch for this issue doesn't contradict your original patch to make the mappings configurable. IMO we should do both.
        Hide
        Benson Margulies added a comment -

        OK, here's the disconnect on the second part of (3). Before I got here, the code was just assuming that all IMSs used the same strings. And, for all I know, several IMSs do agree on those three strings. So, I preserved this. If you want to put those three mappings into JIRA and leave others empty until someone fills in correct mappings, I'll do that.

        Show
        Benson Margulies added a comment - OK, here's the disconnect on the second part of (3). Before I got here, the code was just assuming that all IMSs used the same strings. And, for all I know, several IMSs do agree on those three strings. So, I preserved this. If you want to put those three mappings into JIRA and leave others empty until someone fills in correct mappings, I'll do that.
        Hide
        Dennis Lundberg added a comment -

        Yes, that's my preference, to keep things as separate as possible.

        If we need the same mappings for Trac, then we should add a TracIMS class that we use for Trac. That way we are one step ahead of where we currently are: all supported IMSes have their own implementation and type mapping.

        Show
        Dennis Lundberg added a comment - Yes, that's my preference, to keep things as separate as possible. If we need the same mappings for Trac, then we should add a TracIMS class that we use for Trac. That way we are one step ahead of where we currently are: all supported IMSes have their own implementation and type mapping.
        Hide
        Benson Margulies added a comment -

        /Users/benson/asf/mvn/plugins/maven-changes-plugin svn log -r1134984
        ------------------------------------------------------------------------
        r1134984 | bimargulies | 2011-06-12 17:08:34 -0400 (Sun, 12 Jun 2011) | 3 lines

        MCHANGES-259 Start to build some modularity that knows that we have multiple
        issue management systems at work in here. This is just the beginning.

        ------------------------------------------------------------------------
        /Users/benson/asf/mvn/plugins/maven-changes-plugin

        Show
        Benson Margulies added a comment - /Users/benson/asf/mvn/plugins/maven-changes-plugin svn log -r1134984 ------------------------------------------------------------------------ r1134984 | bimargulies | 2011-06-12 17:08:34 -0400 (Sun, 12 Jun 2011) | 3 lines MCHANGES-259 Start to build some modularity that knows that we have multiple issue management systems at work in here. This is just the beginning. ------------------------------------------------------------------------ /Users/benson/asf/mvn/plugins/maven-changes-plugin
        Benson Margulies made changes -
        Resolution Fixed [ 1 ]
        Status Open [ 1 ] Closed [ 6 ]
        Fix Version/s 2.6 [ 17375 ]
        Mark Thomas made changes -
        Project Import Sun Apr 05 09:14:45 UTC 2015 [ 1428225285377 ]
        Mark Thomas made changes -
        Workflow jira [ 12718093 ] Default workflow, editable Closed status [ 12749430 ]
        Mark Thomas made changes -
        Project Import Sun Apr 05 22:40:15 UTC 2015 [ 1428273615853 ]
        Mark Thomas made changes -
        Workflow jira [ 12955526 ] Default workflow, editable Closed status [ 12992691 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Closed Closed
        7d 4h 28m 1 Benson Margulies 12/Jun/11 17:09

          People

          • Assignee:
            Benson Margulies
            Reporter:
            Benson Margulies
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development