Add support for regex pattern in folders #41

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

src/main/java/jenkins/advancedqueue/jobinclusion/strategy/FolderBasedJobInclusionStrategy.java

 	@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

src/main/java/jenkins/advancedqueue/jobinclusion/strategy/FolderBasedJobInclusionStrategy.java

@@ -62,19 +66,70 @@ public ListBoxModel getListFolderItems() {

 	};

+	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

...ces/jenkins/advancedqueue/jobinclusion/strategy/FolderBasedJobInclusionStrategy/config.jelly

@@ -4,6 +4,11 @@
 			<j:forEach var="folder" items="${descriptor.listFolderItems}">
 				<f:option value="${folder.value}" selected="${folder.value==instance.folderName}">${folder.name}</f:option>
 			</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

src/main/java/jenkins/advancedqueue/jobinclusion/strategy/FolderBasedJobInclusionStrategy.java

+				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

src/main/java/jenkins/advancedqueue/jobinclusion/strategy/FolderBasedJobInclusionStrategy.java

+				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

src/main/java/jenkins/advancedqueue/jobinclusion/strategy/FolderBasedJobInclusionStrategy.java

+
+	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

src/main/java/jenkins/advancedqueue/jobinclusion/strategy/FolderBasedJobInclusionStrategy.java

+		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

src/main/java/jenkins/advancedqueue/jobinclusion/strategy/FolderBasedJobInclusionStrategy.java

@@ -62,19 +66,70 @@ public ListBoxModel getListFolderItems() {

 	};

+	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

src/main/java/jenkins/advancedqueue/jobinclusion/strategy/FolderBasedJobInclusionStrategy.java

 	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

...ces/jenkins/advancedqueue/jobinclusion/strategy/FolderBasedJobInclusionStrategy/config.jelly

@@ -4,6 +4,11 @@
 			<j:forEach var="folder" items="${descriptor.listFolderItems}">
 				<f:option value="${folder.value}" selected="${folder.value==instance.folderName}">${folder.name}</f:option>
 			</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}" />


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

Remove unused logger

d2c87b5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for regex pattern in folders #41

Add support for regex pattern in folders #41

rskupnik commented Jun 26, 2017

oleg-nenashev left a comment

oleg-nenashev Jul 9, 2017

rskupnik Jul 10, 2017

oleg-nenashev Jul 10, 2017

oleg-nenashev Jul 9, 2017

rskupnik Jul 10, 2017

oleg-nenashev Jul 10, 2017

oleg-nenashev Jul 10, 2017

rskupnik Jul 10, 2017

oleg-nenashev Jul 10, 2017

oleg-nenashev Jul 9, 2017

oleg-nenashev Oct 28, 2017

oleg-nenashev Jul 9, 2017

rskupnik Jul 10, 2017

oleg-nenashev Jul 9, 2017

oleg-nenashev Jul 10, 2017

oleg-nenashev Jul 10, 2017

oleg-nenashev Jul 10, 2017

rskupnik commented Jul 19, 2017

oleg-nenashev commented Jul 19, 2017

oleg-nenashev left a comment

oleg-nenashev Oct 28, 2017

oleg-nenashev Oct 28, 2017

		@@ -62,19 +66,70 @@ public ListBoxModel getListFolderItems() {

		};

		static public class JobPattern {

		private String folderName;

		private boolean useJobFilter = false;

Add support for regex pattern in folders #41

Are you sure you want to change the base?

Add support for regex pattern in folders #41

Conversation

rskupnik commented Jun 26, 2017

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rskupnik commented Jul 19, 2017

oleg-nenashev commented Jul 19, 2017

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment