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.

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

Add support for regex pattern in folders #41

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

Conversation

rskupnik
Copy link

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.

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

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

@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

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

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

decisionLogger.addDecisionLog(2, "Using filter ...");
LOGGER.info("Using filter ...");
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.

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?

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

private Pattern getCompiledPattern() {
if (compiledPattern == null)
compiledPattern = Pattern.compile(jobPattern);
Copy link
Member

Choose a reason for hiding this comment

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

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

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

@@ -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.

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

@rskupnik
Copy link
Author

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

@oleg-nenashev
Copy link
Member

I will try to review it on the weekend

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

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

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

@@ -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.

fixed now

@oleg-nenashev oleg-nenashev self-assigned this Oct 28, 2017
@MarkEWaite MarkEWaite added the feature Used by release drafter label Nov 27, 2021
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Apr 15, 2023
@MarkEWaite MarkEWaite requested a review from a team as a code owner June 21, 2024 01:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation feature Used by release drafter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants