Add support for regex pattern in folders by rskupnik · Pull Request #41 · jenkinsci/priority-sorter-plugin

rskupnik · 2017-06-26T08:36:04Z

Adds the capability to filter jobs by regex when using the "by folder" option instead of "by view" option. Works the same as in case of views.

oleg-nenashev

It cannot be accepted in the current state, because such filtering causes a significant performance degradation. Log messages should be also cleaned up.

oleg-nenashev · 2017-07-09T15:18:13Z

+
 	@DataBoundConstructor
-	public FolderBasedJobInclusionStrategy(String folderName) {
+	public FolderBasedJobInclusionStrategy(String folderName, JobPattern jobFilter) {


It breaks the binary compatibility. The original constructor should be retained

I'm not overly familiar with Jenkins' ecosystem (yet) - is it enough to retain the original constructor without the @DataBoundConstructor ?

oleg-nenashev · 2017-07-09T15:19:50Z


 	};

+	static public class JobPattern {


If you want to create a new class, make it describable and use optionalProperty. OTOH I am not sure it is required in this case

Is it needed? I've been looking at how it's implemented in ViewBasedJobInclusionStrategy and it doesn't seem to be required there.

In the new implementation I still do not see much reason to have the class in the current state

I've been looking at how it's implemented in ViewBasedJobInclusionStrategy and it doesn't seem to be required there.

Well, I was not an author of that :( I rather suggest a proper design from the Jenkins standpoint. The current implementation is rather a hack based on the JSON structure presumptions. It should work, but I am not sure if we can guarantee compatibility of it in the Jenkins core

If you want to keep the class as is, mark it as @Restricted(NoExternalUse.class) to prevent the external usage. But it will also require the constructor modification then.

I've tried to make it work with a normal String instead of the JobPattern but to no avail, it seems like I'm missing something obvious here, probably due to my low understanding of the ecosystem. I had to go with the @Restricted route. I'm not sure what constructor modifications you mean, though.

OK. I will probably just create a pull request on the top of your changes

oleg-nenashev · 2017-07-09T15:20:03Z

 			</j:forEach>
+			<f:optionalBlock name="jobFilter" checked="${instance.useJobFilter}" title="Use a regular expression to only include a subset of the included Jobs">
+			    <f:entry title="Regular Expression">
+			        <f:textbox name="jobPattern" field="jobPattern" value="${instance.jobPattern}" />


help file is missing

oleg-nenashev · 2017-07-09T15:22:14Z

+				decisionLogger.addDecisionLog(2, "Using filter ...");
+				LOGGER.info("Using filter ...");
+				try {
+					if (job.getName().matches(getJobPattern())) {


Such approach will require creation of the regex pattern every time. It is inefficient, especially when it comes to the Queue-locking logic. It it better to compile pattern only once and cache it as a transient field of JobPattern

I've changed this, however, I was following the way the same functionality is implemented in ViewBasedJobInclusionStrategy - therefore the inefficiency you mentioned might be present in the ViewBased... implementation as well, if I'm not wrong.

oleg-nenashev · 2017-07-09T15:23:53Z

+				return true;
+			} else {
+				decisionLogger.addDecisionLog(2, "Using filter ...");
+				LOGGER.info("Using filter ...");


Such general-purpose logging provides few info to the admin, because it does not reference job. INFO level is also too high, maybe CONFIG or FINEST?

Restore the original constructor Make the pattern compile only once and re-use later Provide a help file

oleg-nenashev · 2017-07-10T10:31:37Z

+
+	private Pattern getCompiledPattern() {
+		if (compiledPattern == null)
+			compiledPattern = Pattern.compile(jobPattern);


Handle PatternSyntaxException somehow? Or at least add it to throws. If you follow the nullable field advice, it will need an additional check here as well

oleg-nenashev · 2017-07-10T10:33:31Z

+		this.folderName = folderName;
+		this.useJobFilter = (jobFilter != null);
+		if (this.useJobFilter) {
+			this.jobPattern = jobFilter.jobPattern;


Empty pattern from config will override the default value. I would rather make the field nullable, get rid of the default, add @CheckForNull annotation to getter methods. Once the value is null, you can just skip regex check to get a better performance

oleg-nenashev · 2017-07-10T10:35:12Z


 	};

+	static public class JobPattern {


In the new implementation I still do not see much reason to have the class in the current state

rskupnik · 2017-07-19T07:06:11Z

Hey, are there are any more changes required to accept this feature?

oleg-nenashev · 2017-07-19T07:13:02Z

I will try to review it on the weekend

oleg-nenashev

Sorry for not reviewing it timely. The pull request looks good to me, I will fix minor things before the release

oleg-nenashev · 2017-10-28T10:42:54Z

+
 	private String folderName;

+	private boolean useJobFilter = false;


Actually there is no need to keep and persist this flag. jobPattern != null should be enough

oleg-nenashev · 2017-10-28T10:44:35Z

 			</j:forEach>
+			<f:optionalBlock name="jobFilter" checked="${instance.useJobFilter}" title="Use a regular expression to only include a subset of the included Jobs">
+			    <f:entry title="Regular Expression">
+			        <f:textbox name="jobPattern" field="jobPattern" value="${instance.jobPattern}" />


felipecrs · 2025-06-12T00:23:44Z

This feature would be super. I demonstrated how this limitation can be worked around using views instead:

Demonstrate workaround for Priority Sorter lack of regex support felipecrs/docker-images#424

Which can be used while this doesn't land.

Add support for regex pattern in folders

4ddcf07

oleg-nenashev requested changes Jul 9, 2017

View reviewed changes

Improve the implementation

a3ea8d1

Restore the original constructor Make the pattern compile only once and re-use later Provide a help file

oleg-nenashev reviewed Jul 10, 2017

View reviewed changes

rskupnik added 2 commits July 10, 2017 14:40

Add @restricted annotation and protect jobPattern with @checkfornull

4ec1ab8

Fix a logic error when checking for null jobPattern

7ee4bb7

oleg-nenashev approved these changes Oct 28, 2017

View reviewed changes

oleg-nenashev self-assigned this Oct 28, 2017

MarkEWaite added the feature Used by release drafter label Nov 27, 2021

MarkEWaite added 4 commits November 28, 2021 05:02

Merge branch 'master' into regex-folders

67020ee

Fix spotbugs warning for possible NPE

8670d0c

Merge branch 'master' into regex-folders

9a2aea9

Remove redundant null check

b150b21

github-actions Bot added the documentation Improvements or additions to documentation label Apr 15, 2023

MarkEWaite added 5 commits April 14, 2023 21:32

Merge branch 'master' into regex-folders

4ed7cfc

Use spotbugs CheckForNull, format source

ed3313e

Fix merge error

1151f98

Merge branch 'master' into regex-folders

35ce6b2

Merge branch 'master' into regex-folders

7d3d3f4

MarkEWaite requested a review from a team as a code owner June 21, 2024 01:32

MarkEWaite added 4 commits June 20, 2024 19:38

Remove unused logger

d2c87b5

Merge branch 'master' into regex-folders

91e5540

Merge branch 'master' into regex-folders

fbfcc63

Adapt new tests to regex filtering message

aad3403

github-actions Bot added the tests Automated test addition or improvement label May 6, 2025


		private String folderName;

		private boolean useJobFilter = false;

Uh oh!

Conversation

rskupnik commented Jun 26, 2017

Uh oh!

oleg-nenashev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rskupnik commented Jul 19, 2017

Uh oh!

oleg-nenashev commented Jul 19, 2017

Uh oh!

oleg-nenashev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipecrs commented Jun 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants