Skip to content

Support specification of variable patterns #308

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

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

Support specification of variable patterns #308

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6d87fe6
Support specification of variable patterns
rburgstaller Feb 10, 2017
e52b82c
implemented review comments, improved env var handling
rburgstaller Feb 21, 2017
e54aa56
implemented further review comments
rburgstaller Mar 1, 2017
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
69 changes: 58 additions & 11 deletions ...main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTrigger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
import com.sonymobile.tools.gerrit.gerritevents.dto.events.RefUpdated;
import com.sonymobile.tools.gerrit.gerritevents.dto.rest.Notify;

import hudson.EnvVars;
import hudson.Extension;
import hudson.ExtensionList;
import hudson.Util;
Expand All @@ -85,8 +86,12 @@
import hudson.model.Queue;
import hudson.model.Run;
import hudson.model.Result;
import hudson.slaves.EnvironmentVariablesNodeProperty;
import hudson.slaves.NodeProperty;
import hudson.slaves.NodePropertyDescriptor;
import hudson.triggers.Trigger;
import hudson.triggers.TriggerDescriptor;
import hudson.util.DescribableList;
import hudson.util.FormValidation;
import hudson.util.ListBoxModel;
import hudson.util.ListBoxModel.Option;
Expand Down Expand Up @@ -178,6 +183,9 @@ public class GerritTrigger extends Trigger<Job> {
private List<PluginGerritEvent> triggerOnEvents;
private boolean dynamicTriggerConfiguration;
private String triggerConfigURL;
private final EnvVars envVars;
Copy link
Member

@rsandell rsandell Mar 14, 2017

Choose a reason for hiding this comment

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

This is no good, when the project is saved these fields will be serialized with the trigger and take up unnecessary disk space.

I think a ThreadLocal LoadingCache or something is better.

private int globalHashVarsHash;
private int systemEnvVarsHash;

private GerritTriggerTimerTask gerritTriggerTimerTask;
private GerritTriggerInformationAction triggerInformationAction;
Expand All @@ -194,8 +202,10 @@ public GerritTrigger(List<GerritProject> gerritProjects) {
this.skipVote = new SkipVote(false, false, false, false);
this.escapeQuotes = true;
this.serverName = ANY_SERVER;
this.envVars = initEnvVars();

try {
DescriptorImpl descriptor = (DescriptorImpl)getDescriptor();
DescriptorImpl descriptor = (DescriptorImpl) getDescriptor();
if (descriptor != null) {
ListBoxModel options = descriptor.doFillNotificationLevelItems(this.serverName);
if (!options.isEmpty()) {
Expand Down Expand Up @@ -334,6 +344,23 @@ public GerritTrigger(List<GerritProject> gerritProjects, SkipVote skipVote, Inte
this.gerritTriggerTimerTask = null;
this.triggerInformationAction = new GerritTriggerInformationAction();
this.notificationLevel = notificationLevel;
this.envVars = initEnvVars();
}

/**
* Initializes the environment variables used for expansion.
*/
private EnvVars initEnvVars() {
EnvVars result = new EnvVars(EnvVars.masterEnvVars);
DescribableList<NodeProperty<?>, NodePropertyDescriptor> globalProps =
Jenkins.getActiveInstance().getGlobalNodeProperties();
EnvironmentVariablesNodeProperty envVarsProp = globalProps.get(EnvironmentVariablesNodeProperty.class);
EnvVars globalEnvVars = envVarsProp.getEnvVars();
result.putAll(globalEnvVars);

systemEnvVarsHash = EnvVars.masterEnvVars.hashCode();
globalHashVarsHash = globalEnvVars.hashCode();
return result;
}

/**
Expand Down Expand Up @@ -960,26 +987,27 @@ public boolean equals(Object obj) {
}
logger.trace("entering isInteresting projects configured: {} the event: {}", allGerritProjects.size(), event);

updateEnvVarsIfNecessary();
for (GerritProject p : allGerritProjects) {
try {
if (event instanceof ChangeBasedEvent) {
ChangeBasedEvent changeBasedEvent = (ChangeBasedEvent)event;
if (isServerInteresting(event)
&& p.isInteresting(changeBasedEvent.getChange().getProject(),
changeBasedEvent.getChange().getBranch(),
changeBasedEvent.getChange().getTopic())) {
&& p.isInteresting(changeBasedEvent.getChange().getProject(),
changeBasedEvent.getChange().getBranch(),
changeBasedEvent.getChange().getTopic(), envVars)) {

boolean containsFilePathsOrForbiddenFilePaths =
((p.getFilePaths() != null && p.getFilePaths().size() > 0)
|| (p.getForbiddenFilePaths() != null && p.getForbiddenFilePaths().size() > 0));

if (isFileTriggerEnabled() && containsFilePathsOrForbiddenFilePaths) {
if (isServerInteresting(event)
&& p.isInteresting(changeBasedEvent.getChange().getProject(),
changeBasedEvent.getChange().getBranch(),
changeBasedEvent.getChange().getTopic(),
changeBasedEvent.getFiles(
new GerritQueryHandler(getServerConfig(event))))) {
&& p.isInteresting(changeBasedEvent.getChange().getProject(),
changeBasedEvent.getChange().getBranch(),
changeBasedEvent.getChange().getTopic(),
changeBasedEvent.getFiles(
new GerritQueryHandler(getServerConfig(event))), envVars)) {
logger.trace("According to {} the event is interesting.", p);
return true;
}
Expand All @@ -991,20 +1019,39 @@ public boolean equals(Object obj) {
} else if (event instanceof RefUpdated) {
RefUpdated refUpdated = (RefUpdated)event;
if (isServerInteresting(event) && p.isInteresting(refUpdated.getRefUpdate().getProject(),
refUpdated.getRefUpdate().getRefName(), null)) {
refUpdated.getRefUpdate().getRefName(), null, envVars)) {
logger.trace("According to {} the event is interesting.", p);
return true;
}
}
} catch (PatternSyntaxException pse) {
logger.error(MessageFormat.format("Exception caught for project {0} and pattern {1}, message: {2}",
new Object[]{job.getName(), p.getPattern(), pse.getMessage()}));
new Object[]{job.getName(), p.getPattern(), pse.getMessage()}));
}
}
logger.trace("Nothing interesting here, move along folks!");
return false;
}

/**
* Checks if the current system env vars are still the same and updates our local copy if necessary
*/
private synchronized void updateEnvVarsIfNecessary() {
EnvironmentVariablesNodeProperty envVarsProp =
Jenkins.getActiveInstance().getGlobalNodeProperties().get(EnvironmentVariablesNodeProperty.class);
EnvVars globalEnvVars = envVarsProp.getEnvVars();
int newSystemEnvVarsHash = EnvVars.masterEnvVars.hashCode();
int newGlobalEnvVarsHash = globalEnvVars.hashCode();

if (newSystemEnvVarsHash != systemEnvVarsHash || newGlobalEnvVarsHash != globalHashVarsHash) {
this.envVars.clear();
Copy link
Member

@rsandell rsandell Feb 28, 2017

Choose a reason for hiding this comment

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

is EnvVars thread safe? There are multiple threads accessing this code, so you risk them overwriting eachother.

Copy link
Author

@rburgst rburgst Mar 1, 2017

Choose a reason for hiding this comment

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

very good point. It extends TreeMap so unfortunately its not. Is there a performance penalty to consider or is it enough to make it synchronized?

this.envVars.putAll(EnvVars.masterEnvVars);
this.envVars.putAll(globalEnvVars);
systemEnvVarsHash = newSystemEnvVarsHash;
globalHashVarsHash = newGlobalEnvVarsHash;
}
}

/**
* Check whether the event provider contains the same server name as the serverName field.
*
Expand Down
6 changes: 4 additions & 2 deletions src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/data/Branch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
*/
package com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.data;

import hudson.EnvVars;
import hudson.Extension;
import hudson.model.AbstractDescribableImpl;
import hudson.model.Descriptor;
Expand Down Expand Up @@ -90,10 +91,11 @@ public void setPattern(String pattern) {
/**
* Tells if the given branch is matched by this rule.
* @param branch the branch
* @param envVars the environment variables exisiting on the jenkins host.
* @return true if the branch matches.
*/
public boolean isInteresting(String branch) {
return compareType.matches(pattern, branch);
public boolean isInteresting(String branch, EnvVars envVars) {
return compareType.matches(pattern, branch, envVars);
}

/**
Expand Down
9 changes: 6 additions & 3 deletions ...n/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/data/CompareType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
import com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.data.CompareUtil.AntCompareUtil;
import com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.data.CompareUtil.PlainCompareUtil;
import com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.data.CompareUtil.RegExpCompareUtil;
import hudson.EnvVars;

import java.util.LinkedList;
import java.util.List;

Expand Down Expand Up @@ -88,7 +90,7 @@ public static CompareType findByOperator(char operator) {
return PLAIN;
}

private CompareUtil util;
CompareUtil util;
Copy link
Member

@rsandell rsandell Feb 17, 2017

Choose a reason for hiding this comment

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

should be no need for this.

Copy link
Author

@rburgst rburgst Feb 17, 2017

Choose a reason for hiding this comment

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

with the existing setup it was necessary for the test. With your suggestion it won't be any more. Correct.


/**
* Private Constructor.
Expand All @@ -102,10 +104,11 @@ private CompareType(CompareUtil util) {
* Tells if the given string matches the given pattern based on the algorithm of this CompareType instance.
* @param pattern the pattern
* @param str the string
* @param envVars the environment variables exisiting on the jenkins host.
Copy link
Member

@rsandell rsandell Mar 14, 2017

Choose a reason for hiding this comment

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

The old signature needs to be kept and deprecated for backwards binary compatibility

* @return true if the string matches the pattern.
*/
public boolean matches(String pattern, String str) {
return util.matches(pattern, str);
public boolean matches(String pattern, String str, EnvVars envVars) {
return util.matches(pattern, str, envVars);
}

/**
Expand Down
60 changes: 49 additions & 11 deletions ...n/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/data/CompareUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,14 @@
*/
package com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.data;

import java.io.File;
import hudson.EnvVars;
import org.apache.tools.ant.types.selectors.SelectorUtils;

import java.io.File;

/**
* Base interface for the compare-algorithms.
*
* @author Robert Sandell &lt;[email protected]&gt;
*/
public interface CompareUtil {
Expand All @@ -37,9 +40,19 @@ public interface CompareUtil {
* @param pattern the pattern to use.
* @param str the string to match on.
* @return true if the string matches the pattern.
* @deprecated use {@link #matches(String, String, EnvVars)} instead.
*/
boolean matches(String pattern, String str);
Copy link
Member

@rsandell rsandell Mar 14, 2017

Choose a reason for hiding this comment

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

Add a @Deprecated annotation as well so tools has an easier time figuring it out.


/**
* Tells if the given pattern matches the string according to the implemented comparer/algorithm.
* @param pattern the pattern to use.
* @param str the string to match on.
* @param envVars the environment variables exisiting on the jenkins host. Can be {@code null}.
* @return true if the string matches the pattern.
*/
boolean matches(String pattern, String str, EnvVars envVars);
Copy link
Member

@rsandell rsandell Feb 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Darn it, I didn't see that this was an interface when I recommended the change to the signatures, but I did say deprecate the old signature and add a new one.
Since it is a public interface we can't easily tell if there are any implementations of this in other plugins and we need to try to keep binary backwards compatibility.

The simplest way I can think of at the moment would be to

  • Add back boolean matches(String pattern, String str); and deprecate it.
  • Add a new class: AbstractCompareUtil (implements CompareUtil) or something and make it an abstract class, implementing the old method signature that only calls the new method boolean matches(String pattern, String str, EnvVars envVars); with null for envVars.
  • Make AntCompareUtil et.al. extend AbstractCompareUtil instead of implements CompareUtil. They also needs to stay in this class so they keep their old names.
  • Then make all references to CompareUtil in this plugin reference AbstractCompareUtil instead.

Copy link
Author

@rburgst rburgst Mar 1, 2017

Choose a reason for hiding this comment

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

I didn't realise that this code might be used by other plugins, I guess there isn't really a way to figure this out?

Copy link
Member

@rsandell rsandell Mar 1, 2017

Choose a reason for hiding this comment

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

Yes, sorry. The "darn it" was on me not on you :)
This plugin has been around for quite a long time so it's really hard to tell what public methods can be changed and not, not just other plugins but whatever groovy scripts that admins has laying around etc. So we try to keep binary compatibility as much as possible and only make breaking changes when absolutely nessesary.


/**
* Returns the human-readable name of the util.
* @return the name.
Expand All @@ -52,20 +65,42 @@ public interface CompareUtil {
*/
char getOperator();

abstract class AbstractCompareUtil implements CompareUtil {
@Override
public boolean matches(String pattern, String str) {
return matches(pattern, str, null);
}

/**
* Expands the pattern in case it contains tokens that can be resolved with the given envVars.
* @param pattern the pattern to expand.
* @param envVars the envVars to use for expansion, if {@code null} then the unmodified pattern will be returned
* @return the expanded string, if no envVars were provided will return the initial unmodified pattern
*/
protected String expandWithEnvVarsIfPossible(String pattern, EnvVars envVars) {
if (envVars != null) {
return envVars.expand(pattern);
} else {
return pattern;
}
}
}

/**
* Compares based on Ant-style paths.
* like <code>my/&#042;&#042;/something&#042;.git</code>
*/
static class AntCompareUtil implements CompareUtil {
class AntCompareUtil extends AbstractCompareUtil {
Copy link
Member

@rsandell rsandell Mar 14, 2017

Choose a reason for hiding this comment

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

static class


@Override
public boolean matches(String pattern, String str) {
public boolean matches(String pattern, String str, EnvVars envVars) {
// Replace the Git directory separator character (always '/')
// with the platform specific directory separator before
// invoking Ant's platform specific path matching.
String safePattern = pattern.replace('/', File.separatorChar);
String expandedPattern = expandWithEnvVarsIfPossible(safePattern, envVars);
Copy link
Member

@rsandell rsandell Mar 14, 2017

Choose a reason for hiding this comment

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

I think it is better to do this before the \ to / replacement in case any of the used env vars contains platform specific paths.

String safeStr = str.replace('/', File.separatorChar);
return SelectorUtils.matchPath(safePattern, safeStr);
return SelectorUtils.matchPath(expandedPattern, safeStr);
}

@Override
Expand All @@ -80,13 +115,15 @@ public char getOperator() {
}

/**
* Compares with pattern.equals(str).
* Compares with pattern.equalsIgnoreCase(str).
*/
static class PlainCompareUtil implements CompareUtil {
class PlainCompareUtil extends AbstractCompareUtil {

@Override
public boolean matches(String pattern, String str) {
return pattern.equalsIgnoreCase(str);
public boolean matches(String pattern, String str, EnvVars envVars) {
String expandedPattern = expandWithEnvVarsIfPossible(pattern, envVars);

return expandedPattern.equalsIgnoreCase(str);
}

@Override
Expand All @@ -105,11 +142,12 @@ public char getOperator() {
* string.matches(pattern)
* @see java.util.regex.Pattern
*/
static class RegExpCompareUtil implements CompareUtil {
class RegExpCompareUtil extends AbstractCompareUtil {

@Override
public boolean matches(String pattern, String str) {
return str.matches(pattern);
public boolean matches(String pattern, String str, EnvVars envVars) {
String expandedPattern = expandWithEnvVarsIfPossible(pattern, envVars);
return str.matches(expandedPattern);
}

@Override
Expand Down
6 changes: 4 additions & 2 deletions ...main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/data/FilePath.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
*/
package com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.data;

import hudson.EnvVars;
import hudson.Extension;
import hudson.model.AbstractDescribableImpl;
import hudson.model.Descriptor;
Expand Down Expand Up @@ -92,11 +93,12 @@ public void setPattern(String pattern) {
/**
* Tells if the given files are matched by this rule.
* @param files the files in the patch set.
* @param envVars the environment variables exisiting on the jenkins host.
* @return true if the files match.
*/
public boolean isInteresting(List<String> files) {
public boolean isInteresting(List<String> files, EnvVars envVars) {
for (String file : files) {
if (compareType.matches(pattern, file)) {
if (compareType.matches(pattern, file, envVars)) {
return true;
}
}
Expand Down
Loading