Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-10449

Excluded regions do not exclude commits from changelog

    XMLWordPrintable

Details

    Description

      When using Excluded Regions to exclude certain parts of a repository from triggering a build (because it does not affect the build), the commits performed to those excluded regions are still listed in the changelog. If this is a feature and not a bug, it'd be great to at least have an option to exclude them from the changelog.

      Attachments

        Activity

          jorgenpt Jørgen Tjernø created issue -
          eleonov Eugene Leonov added a comment - - edited

          Of course it also relates to "Included Regions" field! Perhaps "Excluded Users" and "Excluded Commit Message" too, it's up to you.

          There is another reason to resolve this issue: JIRA Plugin writes comments to all issues from all commits in "Changes". I don't sure, perhaps Jira Plugin gets issue-list from commits itself (without using "changes" field). If so, I'll create separate issue with "jira" component. Could anybody answer me?

          eleonov Eugene Leonov added a comment - - edited Of course it also relates to "Included Regions" field! Perhaps "Excluded Users" and "Excluded Commit Message" too, it's up to you. There is another reason to resolve this issue: JIRA Plugin writes comments to all issues from all commits in "Changes". I don't sure, perhaps Jira Plugin gets issue-list from commits itself (without using "changes" field). If so, I'll create separate issue with "jira" component. Could anybody answer me?
          lhotari lhotari added a comment -

          This also affects the git plugin.

          lhotari lhotari added a comment - This also affects the git plugin.
          batkinson Brent Atkinson added a comment - - edited

          It seems like it should be a simple matter of iterating through the changes generated for the changelog, and filtering them based on the same rules for the detection check. However, this is more complex than it seems at first.

          The detection happens quite separately from the changelog generation. The detection happens in a private inner class in husdon.scm.SubversionSCM$SVNLogHandler.checkEntry, the handler is coupled to SVNKit's SVNLogEntry class. The changelog generation has its own LogEntry class, and there are a lot of private methods in SubversionSCM that it depends on, so you can't simply factor out the same code to filter them.

          batkinson Brent Atkinson added a comment - - edited It seems like it should be a simple matter of iterating through the changes generated for the changelog, and filtering them based on the same rules for the detection check. However, this is more complex than it seems at first. The detection happens quite separately from the changelog generation. The detection happens in a private inner class in husdon.scm.SubversionSCM$SVNLogHandler.checkEntry, the handler is coupled to SVNKit's SVNLogEntry class. The changelog generation has its own LogEntry class, and there are a lot of private methods in SubversionSCM that it depends on, so you can't simply factor out the same code to filter them.

          I ended up figuring out a way to cleanly unify the filtering. See https://github.com/jenkinsci/subversion-plugin/pull/27

          batkinson Brent Atkinson added a comment - I ended up figuring out a way to cleanly unify the filtering. See https://github.com/jenkinsci/subversion-plugin/pull/27
          kutzi kutzi added a comment -

          I'm not sure that this is a feature every user wants to have.
          I for my case would like to see the whole changelog even if not every commit would trigger a new build.
          So I think this should be configurable.

          BTW: if you're in a situation like these and need to exclude certain regions, it might be a signal that your subversion repository should be modularized.

          kutzi kutzi added a comment - I'm not sure that this is a feature every user wants to have. I for my case would like to see the whole changelog even if not every commit would trigger a new build. So I think this should be configurable. BTW: if you're in a situation like these and need to exclude certain regions, it might be a signal that your subversion repository should be modularized.

          I agree, that was my first reaction as well. If you're doing this systematically it will be an uphill battle and definitely suggests you're fighting your tooling. However, when considering the reasons to exclude parts of the tree from triggering a build, I couldn't generate reasonable arguments for including those changes in the changelog. If the commits will be noise for the builds, it seems to follow that they'll be noise in changelog as well. That's when I started looking at this differently and started working on a solution.

          Would you mind sharing your rationale? Why would you want to see the logs from excluded regions and not verify the project stability by building? Understanding the common reasons for using exclusions/inclusions might help steer others clear of expecting this.

          batkinson Brent Atkinson added a comment - I agree, that was my first reaction as well. If you're doing this systematically it will be an uphill battle and definitely suggests you're fighting your tooling. However, when considering the reasons to exclude parts of the tree from triggering a build, I couldn't generate reasonable arguments for including those changes in the changelog. If the commits will be noise for the builds, it seems to follow that they'll be noise in changelog as well. That's when I started looking at this differently and started working on a solution. Would you mind sharing your rationale? Why would you want to see the logs from excluded regions and not verify the project stability by building? Understanding the common reasons for using exclusions/inclusions might help steer others clear of expecting this.
          kutzi kutzi added a comment -

          One part of the reasoning is that these changes are actually checked out into the workspace, therefore I'd like to see them.
          Other part is that even if you think that the changes may not be relevant for the build, you may be wrong and suddenly the build starts failing without any changelog entry.

          Also I think that most of the time the additional changelog entries will do no harm, so I just would like to see them for completeness (see point 1

          kutzi kutzi added a comment - One part of the reasoning is that these changes are actually checked out into the workspace, therefore I'd like to see them. Other part is that even if you think that the changes may not be relevant for the build, you may be wrong and suddenly the build starts failing without any changelog entry. Also I think that most of the time the additional changelog entries will do no harm, so I just would like to see them for completeness (see point 1

          Thanks for explaining, I agree with your reasoning and it matches my initial thinking. My personal preference would be to never exclude and use proper sub-trees. However, assuming that there are valid reasons for exclusion, which is suggested by the fact it exists:

          • If it affects the build, it's a misconfiguration to exclude something that matters. You want to run verification on relevant changes.
          • If there are a lot of them, like developer docs needing locality to code, you don't want to be spammed with those changes
          • When excluding noisy edits that aren't functionality related. The full changelog may actually do more to hide changes in noise.
          • Ultimately, you want the misconfiguration fixed because you were wrong to exclude in the first place.

          I can see valid cases where the changes in the excluded area are particularly long and it's unlikely they'll be affecting anything, docs for example. I also can see wanting to have those edits omitted from the stream rather than spamming on the next build. It's the same reasoning, but a simulation of http://svnbook.red-bean.com/nightly/en/svn.advanced.sparsedirs.html

          batkinson Brent Atkinson added a comment - Thanks for explaining, I agree with your reasoning and it matches my initial thinking. My personal preference would be to never exclude and use proper sub-trees. However, assuming that there are valid reasons for exclusion, which is suggested by the fact it exists: If it affects the build, it's a misconfiguration to exclude something that matters. You want to run verification on relevant changes. If there are a lot of them, like developer docs needing locality to code, you don't want to be spammed with those changes When excluding noisy edits that aren't functionality related. The full changelog may actually do more to hide changes in noise. Ultimately, you want the misconfiguration fixed because you were wrong to exclude in the first place. I can see valid cases where the changes in the excluded area are particularly long and it's unlikely they'll be affecting anything, docs for example. I also can see wanting to have those edits omitted from the stream rather than spamming on the next build. It's the same reasoning, but a simulation of http://svnbook.red-bean.com/nightly/en/svn.advanced.sparsedirs.html
          kutzi kutzi added a comment -

          You don't have to convince me that there are lots of good use cases to exclude the changes from the changelog. I'd even say that these use cases are more relevant than my use cases.

          Unfortunately, to exclude the changes from the changelog is a backward incompatible change and should therefore (IMHO) be configurable.
          Who knows, maybe someone else has a perfectly reasonable usecase to not exclude the changes and he would be negatively affected by this change.

          kutzi kutzi added a comment - You don't have to convince me that there are lots of good use cases to exclude the changes from the changelog. I'd even say that these use cases are more relevant than my use cases. Unfortunately, to exclude the changes from the changelog is a backward incompatible change and should therefore (IMHO) be configurable. Who knows, maybe someone else has a perfectly reasonable usecase to not exclude the changes and he would be negatively affected by this change.

          I agree, configuration would make it far less controversial. I apologize if I am belaboring it, but it helps to think it through and document the reasoning. I am definitely not trying to convert you. Like I said, my own preference would be to avoid the complication.

          batkinson Brent Atkinson added a comment - I agree, configuration would make it far less controversial. I apologize if I am belaboring it, but it helps to think it through and document the reasoning. I am definitely not trying to convert you. Like I said, my own preference would be to avoid the complication.
          eleonov Eugene Leonov added a comment -

          Firstsy, guys you are wonderful!
          Secondly, optional filtering is the best way.
          Thirdly, will this solution work for git?

          eleonov Eugene Leonov added a comment - Firstsy, guys you are wonderful! Secondly, optional filtering is the best way. Thirdly, will this solution work for git?
          kutzi kutzi added a comment -

          No, this would only apply for the subversion plugin. The Git plugin is a completely different codebase.

          kutzi kutzi added a comment - No, this would only apply for the subversion plugin. The Git plugin is a completely different codebase.

          The updated pull request now implements optional filtering of the changelog. All existing behavior is left intact, and the default for new configurations is not to filter the changelog.

          batkinson Brent Atkinson added a comment - The updated pull request now implements optional filtering of the changelog. All existing behavior is left intact, and the default for new configurations is not to filter the changelog.

          Refactored the filtering test so it tests both the old and new behavior, should be good to go.

          batkinson Brent Atkinson added a comment - Refactored the filtering test so it tests both the old and new behavior, should be good to go.

          Code changed in jenkins
          User: Brent Atkinson
          Path:
          src/main/java/hudson/scm/DirAwareSVNXMLLogHandler.java
          src/main/java/hudson/scm/SVNLogFilter.java
          src/main/java/hudson/scm/SubversionChangeLogBuilder.java
          src/main/java/hudson/scm/SubversionSCM.java
          http://jenkins-ci.org/commit/subversion-plugin/f71918fc997a38fab6b9133942b8ee68e70c999c
          Log:
          JENKINS-10449 Factored out svnlog filter to use with detection and changelogs

          scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Brent Atkinson Path: src/main/java/hudson/scm/DirAwareSVNXMLLogHandler.java src/main/java/hudson/scm/SVNLogFilter.java src/main/java/hudson/scm/SubversionChangeLogBuilder.java src/main/java/hudson/scm/SubversionSCM.java http://jenkins-ci.org/commit/subversion-plugin/f71918fc997a38fab6b9133942b8ee68e70c999c Log: JENKINS-10449 Factored out svnlog filter to use with detection and changelogs

          Code changed in jenkins
          User: Christoph Kutzinski
          Path:
          src/main/java/hudson/scm/DefaultSVNLogFilter.java
          src/main/java/hudson/scm/DirAwareSVNXMLLogHandler.java
          src/main/java/hudson/scm/NullSVNLogFilter.java
          src/main/java/hudson/scm/SVNLogFilter.java
          src/main/java/hudson/scm/SubversionChangeLogBuilder.java
          src/main/java/hudson/scm/SubversionSCM.java
          src/main/resources/hudson/scm/SubversionSCM/config.jelly
          src/main/resources/hudson/scm/SubversionSCM/help-filterChangelog.html
          src/test/java/hudson/scm/DefaultSVNLogFilterTest.java
          src/test/java/hudson/scm/SubversionSCMTest.java
          src/test/java/hudson/scm/SubversionSCMUnitTest.java
          src/test/resources/hudson/scm/JENKINS-10449.zip
          http://jenkins-ci.org/commit/subversion-plugin/72be6320a61f79ec329e0f42b496dbac8fff1644
          Log:
          Merge pull request #27 from batkinson/JENKINS-10449

          Filter changelog based on the same inclusion/exclusion filters used to detect changes. [FIXED JENKINS-10449]

          Compare: https://github.com/jenkinsci/subversion-plugin/compare/ed8a1f80131f...72be6320a61f

          scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Christoph Kutzinski Path: src/main/java/hudson/scm/DefaultSVNLogFilter.java src/main/java/hudson/scm/DirAwareSVNXMLLogHandler.java src/main/java/hudson/scm/NullSVNLogFilter.java src/main/java/hudson/scm/SVNLogFilter.java src/main/java/hudson/scm/SubversionChangeLogBuilder.java src/main/java/hudson/scm/SubversionSCM.java src/main/resources/hudson/scm/SubversionSCM/config.jelly src/main/resources/hudson/scm/SubversionSCM/help-filterChangelog.html src/test/java/hudson/scm/DefaultSVNLogFilterTest.java src/test/java/hudson/scm/SubversionSCMTest.java src/test/java/hudson/scm/SubversionSCMUnitTest.java src/test/resources/hudson/scm/ JENKINS-10449 .zip http://jenkins-ci.org/commit/subversion-plugin/72be6320a61f79ec329e0f42b496dbac8fff1644 Log: Merge pull request #27 from batkinson/ JENKINS-10449 Filter changelog based on the same inclusion/exclusion filters used to detect changes. [FIXED JENKINS-10449] Compare: https://github.com/jenkinsci/subversion-plugin/compare/ed8a1f80131f...72be6320a61f
          scm_issue_link SCM/JIRA link daemon made changes -
          Field Original Value New Value
          Resolution Fixed [ 1 ]
          Status Open [ 1 ] Resolved [ 5 ]
          rtyler R. Tyler Croy made changes -
          Workflow JNJira [ 140642 ] JNJira + In-Review [ 189167 ]

          People

            Unassigned Unassigned
            jorgenpt Jørgen Tjernø
            Votes:
            6 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: