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

Feature: include/exclude branches of multi-branch pipelines #263

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

Feature: include/exclude branches of multi-branch pipelines #263

wants to merge 2 commits into from

Conversation

reftel
Copy link

@reftel reftel commented Oct 14, 2016

Closes #246

@reftel reftel mentioned this pull request Oct 14, 2016
Open
@reftel reftel force-pushed the feature/multibranch branch 2 times, most recently from 8b73ba9 to a84ebb1 Compare October 17, 2016 06:27
jan-molak
jan-molak requested changes Oct 18, 2016
View reviewed changes
Copy link
Member

@jan-molak jan-molak left a comment

Choose a reason for hiding this comment

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

Hey @reftel and many thanks for your contribution :-) I've provided feedback in the comments.

...r-plugin/src/main/java/com/smartcodeltd/jenkinsci/plugins/buildmonitor/BuildMonitorView.java Outdated

//A little bit of evil to make the type system happy.
@SuppressWarnings("unchecked")
List<ItemGroup<?>> groups = new ArrayList(filter(super.getItems(), AbstractFolder.class));
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the Folder plugin is not installed and the class is not available?

Copy link
Author

Choose a reason for hiding this comment

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

Good point. Checking via Class.foName and handling ClassNotFoundException.

...r-plugin/src/main/java/com/smartcodeltd/jenkinsci/plugins/buildmonitor/BuildMonitorView.java Outdated
List<ItemGroup<?>> groups = new ArrayList(filter(super.getItems(), AbstractFolder.class));

Pattern includePattern = Strings.isNullOrEmpty(currentConfig().getInclude()) ? null : Pattern.compile(currentConfig().getInclude());
Pattern excludePattern = Strings.isNullOrEmpty(currentConfig().getExclude()) ? null : Pattern.compile(currentConfig().getExclude());
Copy link
Member

Choose a reason for hiding this comment

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

Could we avoid all the null checks here and delegate the decision of whether or not a job should be included in the view to some other class?

Copy link
Author

Choose a reason for hiding this comment

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

Refactored out to JobFilter.

...ces/com/smartcodeltd/jenkinsci/plugins/buildmonitor/BuildMonitorView/configure-entries.jelly Outdated
@@ -56,6 +56,14 @@
<f:textbox name="title" value="${it.title}"/>
</f:entry>

<f:entry title="${%Include Branches}" help="${descriptor.getHelpFile('includeBranches')}">
<f:textbox name="include" field="include" value="${it.include()}"/>
Copy link
Member

Choose a reason for hiding this comment

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

Could we call the property includeBranches or branchesToInclude so that the property provides a bit more of a context?

Copy link
Author

Choose a reason for hiding this comment

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

Agreed. Went with branchesToInclude.

...ces/com/smartcodeltd/jenkinsci/plugins/buildmonitor/BuildMonitorView/configure-entries.jelly Outdated
</f:entry>

<f:entry title="${%Exclude Branches}" help="${descriptor.getHelpFile('excludeBranches')}">
<f:textbox name="exclude" field="exclude" value="${it.exclude()}"/>
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

Copy link
Author

Choose a reason for hiding this comment

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

Same as above =)

@reftel
Copy link
Author

reftel commented Oct 18, 2016

Updated as suggested. Also added some tests to show how the include/exclude branch rules work.

@jan-molak
Copy link
Member

Hey @reftel, I've just run the local build (mvn clean verify) and am seeing the acceptance tests failing for all the scenarios that don't use the Folders plugin (such as those here):

2016-10-27 17:52:05:075 +0100 [jenkins] DEBUG JenkinsLogWatcher - Caused by: java.lang.NullPointerException
2016-10-27 17:52:05:075 +0100 [jenkins] DEBUG JenkinsLogWatcher -   at hudson.Util.filter(Util.java:104)
2016-10-27 17:52:05:075 +0100 [jenkins] DEBUG JenkinsLogWatcher -   at com.smartcodeltd.jenkinsci.plugins.buildmonitor.JobFilter.filterJobs(JobFilter.java:37)
2016-10-27 17:52:05:075 +0100 [jenkins] DEBUG JenkinsLogWatcher -   at com.smartcodeltd.jenkinsci.plugins.buildmonitor.BuildMonitorView.jobViews(BuildMonitorView.java:161)
2016-10-27 17:52:05:075 +0100 [jenkins] DEBUG JenkinsLogWatcher -   at com.smartcodeltd.jenkinsci.plugins.buildmonitor.BuildMonitorView.fetchJobViews(BuildMonitorView.java:150)
2016-10-27 17:52:05:075 +0100 [jenkins] DEBUG JenkinsLogWatcher -   ... 61 more

Looks like the filter(...) doesn't like to be given nulls. Would you mind taking a look?

Thanks,
J

@reftel
Copy link
Author

reftel commented Oct 27, 2016

Now handling nulls up to 15% better - or more!

jan-molak
jan-molak reviewed Oct 27, 2016
View reviewed changes
...-monitor-plugin/src/main/java/com/smartcodeltd/jenkinsci/plugins/buildmonitor/JobFilter.java Outdated
return jobs;
}

for (ItemGroup<?> folder : filter(items, abstractFolderClass)) {
Copy link
Member

Choose a reason for hiding this comment

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

Still an NPE if there's no Folder plugin installed. The acceptance tests should give you the same result.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry about that. I haven't gotten any of the acceptance tests passing on master on either of my machines, so it's hard to verify. Improved the handling of the folder class not being present and folders returning null as items, though. Hopefully, that should be it.

@jan-molak
Copy link
Member

Hey @reftel, I just had another look at the PR and it looks like the following acceptance test is now failing due to the JobFilter producing duplicates if there are jobs nested in folders and sub-folders.

I'll try to look into what might be causing this problem.

J

@sazo
Copy link

sazo commented Jul 4, 2017

@jan-molak anything i can do to speed this PR up? :)

@arnemertz
Copy link

@reftel do you have an ETA on this?

@erikhakansson
Copy link
Contributor

@reftel @jan-molak can we have an update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jan-molak jan-molak jan-molak requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@reftel @jan-molak @sazo @arnemertz @erikhakansson