Add new strategy to cancel builds for specific commit messages #20
Conversation
|
This could become a great replacement of the (quite broken) SCM Skip Plugin. |
|
Referring to the problem I wrote about earlier regarding checking the strategies separately (both conditions had to be met to suspend the build execution)... "All" here indicates that all strategies must pass the build to actually run a build, so if any strategy will prevent it, build will be suspended. |
|
@FrogDevelopper Are you a new maintainer of the plugin and would like to take a look at this MR? |
|
@ljackiewicz |
| final SCMSourceOwner owner = source.getOwner(); | ||
| if (owner == null) { | ||
| LOGGER.severe("Error verify SCM source owner"); | ||
| LOGGER.log(Level.SEVERE, "Error verify SCM source owner"); |
There was a problem hiding this comment.
Just for consistency, throughout the whole project the Logger is called by the log method instead of a dedicated method for a specific logging level, e.g. info, warning, severe.
There was a problem hiding this comment.
Why is it better to call LOGGER.XXX(msg) instead of LOGGER.log(XXX,msg)?
And should we then make a similar change to Logger calling throughout the project?
There was a problem hiding this comment.
Why is it better to call LOGGER.XXX(msg) instead of LOGGER.log(XXX,msg)?
I don't know if it is better, or because it was done in a "old" way
|
|
||
| @Override | ||
| Set<String> getExpressions(List<GitChangeSet> changeSets) { | ||
| GitChangeSet lastCommit = changeSets.get(0); |
There was a problem hiding this comment.
one side effect is that when rebasing the branch, you may want to have the CI triggered, even if the last commit match an exclusion message
There was a problem hiding this comment.
That's a good point, so I tried to check whether we have the ability to detect that a rebase has been performed. Even if such operations are usually performed on feature/bugfix branches, etc., and most likely before the PR is created, it is still worth taking it into account.
I know that for the commit object in Git itself, we can retrieve something like author date and committer date, based on which we can detect that "the repository history has been overwritten" in some way, but unfortunately, these values are not available through the GitChangeSet object.
Do you have any idea, or do you know if we can do it another way?
There was a problem hiding this comment.
The only lead I've come across regarding detecting if a rebase has been performed in the repository is the authorOrCommitter parameter of the GitChangeLogParser class used in the BranchBuildStrategyHelper.getGitChangeSetList() method. However, it would require parsing the Git changelog twice to extract changelogs (lists) composed of GitChangeSet objects with date values set to either author or committer date.
So it's not entirely unattainable, but it would most likely require significant changes to the project structure.
|
@ljackiewicz |
|
@FrogDevelopper Sure, I can squash commits, but I don't know if we'll lose the review then. |
ljackiewicz
force-pushed
the
ljackiewicz/exclude-messages-strategy
branch
from
6702587
to
9bee6e1
Compare
no, it is OK, even if we loose the comments |
This PR contains minor code formatting fixes and adds a brand-new strategy to cancel builds for commits that contain a specified phrase in the commit message.
The new strategy is implemented under new classes (not extending AbstractBranchBuildStrategy, which is strongly region-strategies-oriented), so for now region strategies are not impacted in any way (I tried in parallel to merge these two strategies under common codebase on my second branch).
The introduced changes were manually tested on own Jenkins instance for Multibranch pipelines and Organization folder job types (from GitLab Branch Source plugin).
The only problem I've encountered is using both strategies at the same time, because it seems that strategies are checked with an OR condition, so in this case build will only be actual stopped if both conditions are met, not only one of them (e.g. [ci-skip] phrase in commit message and changes of README.md file).
I'm wondering if there's any way to solve this problem other than placing both strategies under a common form (common class), or maybe it's expected behaviors using strategies at all (because it can also have an impact for strategies provided from any other plugins).