Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for regex pattern in folders #41

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Add support for regex pattern in folders #41

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
4ddcf07
Add support for regex pattern in folders
rskupnik Jun 22, 2017
a3ea8d1
Improve the implementation
rskupnik Jul 10, 2017
4ec1ab8
Add @Restricted annotation and protect jobPattern with @CheckForNull
rskupnik Jul 10, 2017
7ee4bb7
Fix a logic error when checking for null jobPattern
rskupnik Jul 10, 2017
67020ee
Merge branch 'master' into regex-folders
MarkEWaite Nov 28, 2021
8670d0c
Fix spotbugs warning for possible NPE
MarkEWaite Nov 28, 2021
9a2aea9
Merge branch 'master' into regex-folders
MarkEWaite Apr 15, 2023
b150b21
Remove redundant null check
MarkEWaite Apr 15, 2023
4ed7cfc
Merge branch 'master' into regex-folders
MarkEWaite Apr 15, 2023
ed3313e
Use spotbugs CheckForNull, format source
MarkEWaite Apr 15, 2023
1151f98
Fix merge error
MarkEWaite Apr 15, 2023
35ce6b2
Merge branch 'master' into regex-folders
MarkEWaite Jul 28, 2023
7d3d3f4
Merge branch 'master' into regex-folders
MarkEWaite Jun 21, 2024
d2c87b5
Remove unused logger
MarkEWaite Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 57 additions & 2 deletions ...ain/java/jenkins/advancedqueue/jobinclusion/strategy/FolderBasedJobInclusionStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
import hudson.util.ListBoxModel;

import java.util.List;
import java.util.logging.Logger;
import java.util.regex.PatternSyntaxException;

import jenkins.advancedqueue.DecisionLogger;
import jenkins.advancedqueue.jobinclusion.JobInclusionStrategy;
Expand All @@ -43,6 +45,8 @@
*/
public class FolderBasedJobInclusionStrategy extends JobInclusionStrategy {

private final static Logger LOGGER = Logger.getLogger(FolderBasedJobInclusionStrategy.class.getName());

@Extension(optional = true)
static public class FolderBasedJobInclusionStrategyDescriptor extends
AbstractJobInclusionStrategyDescriptor<FolderBasedJobInclusionStrategy> {
Expand All @@ -62,19 +66,70 @@ public ListBoxModel getListFolderItems() {

};

static public class JobPattern {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

private String jobPattern;

@DataBoundConstructor
public JobPattern(String jobPattern) {
this.jobPattern = jobPattern;
}

}

private String folderName;

private boolean useJobFilter = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


private String jobPattern = ".*";

@DataBoundConstructor
public FolderBasedJobInclusionStrategy(String folderName) {
public FolderBasedJobInclusionStrategy(String folderName, JobPattern jobFilter) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it is

this.folderName = folderName;
this.useJobFilter = (jobFilter != null);
if (this.useJobFilter) {
this.jobPattern = jobFilter.jobPattern;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

public String getFolderName() {
return folderName;
}

public boolean isUseJobFilter() {
return useJobFilter;
}

public String getJobPattern() {
return jobPattern;
}

@Override
public boolean contains(DecisionLogger decisionLogger, Job<?, ?> job) {
return job.getFullName().startsWith(folderName);
if (job.getFullName().startsWith(folderName)) {
if (!isUseJobFilter() || getJobPattern().trim().isEmpty()) {
decisionLogger.addDecisionLog(2, "Not using filter ...");
LOGGER.info("Not using filter ...");
return true;
} else {
decisionLogger.addDecisionLog(2, "Using filter ...");
LOGGER.info("Using filter ...");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

try {
if (job.getName().matches(getJobPattern())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

decisionLogger.addDecisionLog(3, "Job is matching the filter ...");
LOGGER.info("Job is matching the filter ...");
return true;
} else {
decisionLogger.addDecisionLog(3, "Job is not matching the filter ...");
LOGGER.info("Job is not matching the filter ...");
return false;
}
} catch (PatternSyntaxException e) {
decisionLogger.addDecisionLog(3, "Filter has syntax error");
LOGGER.info("Filter has syntax error");
return false;
}
}
}
return false;
}
}
5 changes: 5 additions & 0 deletions .../jenkins/advancedqueue/jobinclusion/strategy/FolderBasedJobInclusionStrategy/config.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

help file is missing

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed now

</f:entry>
</f:optionalBlock>
</select>
</f:entry>
</j:jelly>