Support specification of variable patterns #308

rburgst · 2017-02-10T21:49:28Z

this change allows the use of system environment patterns for
project and branch specification such as

${PROJECT_NAME}
Example:
the gerrit host has a configured environment variable defined as

export PROJECT_NAME=myproject

now when we push a review commit for "myproject" then the gerrit
trigger will automatically react
one use case for this is to be able to specfiy a generic seed
job for a jenkins instance which will automatically trigger
a build on the variable based project

- this change allows the use of system environment patterns for project and branch specification such as ${PROJECT_NAME} - Example: the gerrit host has a configured environment variable defined as export PROJECT_NAME=myproject now when we push a review commit for "myproject" then the gerrit trigger will automatically react - one use case for this is to be able to specfiy a generic seed job for a jenkins instance which will automatically trigger a build on the variable based project

rsandell

This PR looks incomplete, did you forget to add some files to the commit?

rsandell · 2017-02-17T11:14:54Z

...main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/data/CompareUtil.java

@@ -56,7 +59,7 @@
     * Compares based on Ant-style paths.
     * like <code>my/&#042;&#042;/something&#042;.git</code>
     */
-    static class AntCompareUtil implements CompareUtil {


why remove the static declaration?

IntelliJ improvement suggestion. A static inner class in an interface has no effect.

rsandell · 2017-02-17T11:16:52Z

...main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/data/CompareUtil.java

+     * Compares just like plain comparison, however, patterns are expanded using the
+     * given system environment settings.
+     */
+    class PlainVarCompareUtil implements CompareUtil {


I think it would be better to skip this one and add the functionality to the existing Plain comparer, deprecate the old matches method and introduce a new one that also takes an EnvVars as parameter.

I didn't want to mess too much with the existing code, but this would definitely be a good improvement. I can do that.

rsandell · 2017-02-17T11:18:35Z

...main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/data/CompareType.java

@@ -88,7 +94,7 @@ public static CompareType findByOperator(char operator) {
        return PLAIN;
    }

-    private CompareUtil util;
+    CompareUtil util;


should be no need for this.

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

rsandell · 2017-02-17T11:20:18Z

src/main/webapp/trigger/help-GerritTriggerConfiguration.html

@@ -8,7 +8,9 @@
    You can specify the name pattern in three different ways, as provided by the &quot;Type&quot; drop-down menu.
 </p>
 <ul>
-    <li><strong>Plain:</strong> The exact name in Gerrit, case sensitive equality.</li>
+    <li><strong>Plain:</strong> The exact name in Gerrit, case insensitive equality.</li>
+    <li><strong>PlainVar:</strong> The exact name in Gerrit, case insensitive equality, allows usage of system environment


I doubt it is on the gerrit host? You me4an the Jenkins master right? Hard to say since this PR seems incomplete.

Yes, the variables have to exist on the Jenkins host.

rsandell · 2017-02-17T11:29:21Z

Just tickling the PR builder

rburgst · 2017-02-17T12:36:36Z

My git workspace is clean, what files do you think are missing?

rsandell · 2017-02-17T15:45:43Z

Well, there should be a call to inject other than the test, where else would the environment come from?

rsandell · 2017-02-17T15:49:25Z

...main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/data/CompareUtil.java

+        private final EnvVars systemEnvVars;
+
+        public PlainVarCompareUtil() {
+            systemEnvVars = new EnvVars(EnvVars.masterEnvVars);


Ah, I had missed the use of EnvVars.masterEnvVars. I don't think it's a good choice to put it here since you would then only pick up whatever is set when Jenkins starts up and not what is changed during runtime.
We need to find a way to provide it as close to the call to matches as possible, without having to create a new instance and copy over all values from EnvVars.masterEnvVars for every job check, since that could cause some performance issues.

rsandell · 2017-02-28T15:49:36Z

src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTrigger.java

+        int newSystemEnvVarsHash = EnvVars.masterEnvVars.hashCode();
+        int newGlobalEnvVarsHash = globalEnvVars.hashCode();
+        if (newSystemEnvVarsHash != systemEnvVarsHash || newGlobalEnvVarsHash != globalHashVarsHash) {
+            this.envVars.clear();


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

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?

rsandell · 2017-02-28T16:02:01Z

...main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/data/CompareUtil.java

     * @return true if the string matches the pattern.
     */
-    boolean matches(String pattern, String str);
+    boolean matches(String pattern, String str, EnvVars envVars);


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.

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?

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.

rsandell · 2017-02-28T16:02:34Z

...main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/data/CompareUtil.java

-        public boolean matches(String pattern, String str) {
-            return str.matches(pattern);
+        public boolean matches(String pattern, String str, EnvVars envVars) {
+            String expandedPattern = envVars.expand(pattern);


needs null check on envVars

rsandell · 2017-02-28T16:03:52Z

...in/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/data/GerritProject.java

     * @return true is the rules match.
     */
-    public boolean isInteresting(String project, String branch, String topic, List<String> files) {
-        if (compareType.matches(pattern, project)) {
+    public boolean isInteresting(String project, String branch, String topic, List<String> files, EnvVars envVars) {


Same here. This is a public api and needs to keep backwards compatibility as much as possible.

Keep the old signature, deprecate it and have it call the new signature with null to the new parameter.

My old comment is still valid. Old method signatures needs to be kept and deprecated.

rsandell

The change breaks backwards binary compatibility

- now we synchronize access to the global EnvVars in order to be thread safe - keep original public API and deprecate it

rburgst · 2017-03-01T11:06:06Z

please let me know if the general direction of the patch is fine, then I will proceed and see about the build failures.

rsandell · 2017-03-14T10:24:57Z

...main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/data/CompareType.java

     * @return true if the string matches the pattern.
     */
-    public boolean matches(String pattern, String str) {


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

rsandell · 2017-03-14T10:26:40Z

...main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/data/CompareUtil.java

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


static class

rsandell · 2017-03-14T10:28:00Z

...main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/data/CompareUtil.java

@@ -37,10 +40,20 @@
     * @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);


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

rsandell · 2017-03-14T10:31:44Z

...main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/data/CompareUtil.java

            // 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);


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

rsandell · 2017-03-14T10:33:59Z

...in/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/data/GerritProject.java

     * @return true is the rules match.
     */
-    public boolean isInteresting(String project, String branch, String topic, List<String> files) {
-        if (compareType.matches(pattern, project)) {
+    public boolean isInteresting(String project, String branch, String topic, List<String> files, EnvVars envVars) {


My old comment is still valid. Old method signatures needs to be kept and deprecated.

rsandell · 2017-03-14T10:36:17Z

src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTrigger.java

@@ -178,6 +183,9 @@
    private List<PluginGerritEvent> triggerOnEvents;
    private boolean dynamicTriggerConfiguration;
    private String triggerConfigURL;
+    private final EnvVars envVars;


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.

rsandell requested changes Feb 17, 2017

View reviewed changes

rsandell closed this Feb 17, 2017

rsandell reopened this Feb 17, 2017

rsandell reviewed Feb 17, 2017

View reviewed changes

implemented review comments, improved env var handling

e52b82c

rsandell reviewed Feb 28, 2017

View reviewed changes

rsandell requested changes Feb 28, 2017

View reviewed changes

implemented further review comments

e54aa56

- now we synchronize access to the global EnvVars in order to be thread safe - keep original public API and deprecate it

rsandell requested changes Mar 14, 2017

View reviewed changes

tkgogogo approved these changes Jun 3, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support specification of variable patterns #308

Support specification of variable patterns #308

rburgst commented Feb 10, 2017

rsandell left a comment

rsandell Feb 17, 2017

rburgst Feb 17, 2017

rsandell Feb 17, 2017

rburgst Feb 17, 2017

rsandell Feb 17, 2017

rburgst Feb 17, 2017

rsandell Feb 17, 2017

rburgst Feb 17, 2017

rsandell commented Feb 17, 2017

rburgst commented Feb 17, 2017

rsandell commented Feb 17, 2017

rsandell Feb 17, 2017

rsandell Feb 28, 2017

rburgst Mar 1, 2017

rsandell Feb 28, 2017 •

edited

Loading

rburgst Mar 1, 2017

rsandell Mar 1, 2017

rsandell Feb 28, 2017

rsandell Feb 28, 2017

rsandell Mar 14, 2017

rsandell left a comment

rburgst commented Mar 1, 2017

rsandell Mar 14, 2017

rsandell Mar 14, 2017

rsandell Mar 14, 2017

rsandell Mar 14, 2017

rsandell Mar 14, 2017

rsandell Mar 14, 2017

Support specification of variable patterns #308

Are you sure you want to change the base?

Support specification of variable patterns #308

Conversation

rburgst commented Feb 10, 2017

rsandell 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

rsandell commented Feb 17, 2017

rburgst commented Feb 17, 2017

rsandell commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsandell Feb 28, 2017 • edited Loading

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

rsandell left a comment

Choose a reason for hiding this comment

rburgst commented Mar 1, 2017

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

rsandell Feb 28, 2017 •

edited

Loading