Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-22222

Check variable names as Jenkins core expands

    Details

    • Similar Issues:

      Description

      After the parameterized trigger plugin wants to exchange all choices to extensible choices we have to change many properties in our jobs. But that's OK.

      Problem:
      Because of many of our properties containing dots we have to overwork half of our build process.

      Suggestion:
      Please check if the name of a property can contain . too.


      Extensible Choice Parameter checks parameter names as Jenkins 1.466 expands variable
      (that is, alphabets, numbers, and underscore: https://github.com/jenkinsci/jenkins/blob/jenkins-1.466/core/src/main/java/hudson/Util.java#L111 )

      Jenkins 1.526 gets to allow dots in JENKINS-16660, f664639.

      Extensible Choice Parameter should check parameter names as the Jenkins core it runs on.

        Attachments

          Issue Links

            Activity

            Hide
            zioschild Sven Appenrodt added a comment -

            Hint: getting triggerd by the parameterized trigger plugin whith having a choice "foo.bar" works, only the validation check for this field fails. Didn't tested accessing the choice via e.g. groovy...

            Show
            zioschild Sven Appenrodt added a comment - Hint: getting triggerd by the parameterized trigger plugin whith having a choice "foo.bar" works, only the validation check for this field fails. Didn't tested accessing the choice via e.g. groovy...
            Hide
            ikedam ikedam added a comment -

            It depends on how to use it.
            If used in shell or windows batch, it would does not work correct.
            Current parameter name check is performed under the rule of bash (alphabets, numbers, underscore).

            Though I haven't tested yet, it would work correct in almost other cases even the configuration page displays an error. Groovy scripts would also work well as they access build variables as a map object.

            I'll ask Jenkins developers whether there is a guideline about parameter names, and consider what's the best way.

            For now, I think followings are good for your case.

            • Test parameters with dots work correct in your environments. Error messages in configuration pages are just informational, and Extensible Choice Parameter does not kick those values even in future releases.
              • But I cannot ensure Jenkins itself accepts any values even in future reases...
            • I still recommend you to use bash-like parameter names if you can easily adapt it. There may be a day you want to access those values from bash scripts or windows batches.
            Show
            ikedam ikedam added a comment - It depends on how to use it. If used in shell or windows batch, it would does not work correct. Current parameter name check is performed under the rule of bash (alphabets, numbers, underscore). Though I haven't tested yet, it would work correct in almost other cases even the configuration page displays an error. Groovy scripts would also work well as they access build variables as a map object. I'll ask Jenkins developers whether there is a guideline about parameter names, and consider what's the best way. For now, I think followings are good for your case. Test parameters with dots work correct in your environments. Error messages in configuration pages are just informational, and Extensible Choice Parameter does not kick those values even in future releases. But I cannot ensure Jenkins itself accepts any values even in future reases... I still recommend you to use bash-like parameter names if you can easily adapt it. There may be a day you want to access those values from bash scripts or windows batches.
            Hide
            zioschild Sven Appenrodt added a comment -

            Thanks for asking.
            I assume there is no guideline but asking doesn't cost anything than a little time

            In that case - can we make a compromise that you continue checking the names but display the result (if any) as warning? Maybe many users will missinterpret your "error", but a warning like "use bash like names [a-zA-Z_-], there might be problems with using this parameter in actions" or something like this might be better. It's just to warn the user using that kind of name, not to forbid it.

            Thanks a lot

            Show
            zioschild Sven Appenrodt added a comment - Thanks for asking. I assume there is no guideline but asking doesn't cost anything than a little time In that case - can we make a compromise that you continue checking the names but display the result (if any) as warning? Maybe many users will missinterpret your "error", but a warning like "use bash like names [a-zA-Z_-] , there might be problems with using this parameter in actions" or something like this might be better. It's just to warn the user using that kind of name, not to forbid it. Thanks a lot
            Hide
            ikedam ikedam added a comment -

            I'm not sure now. I know I can take one of follewing ways:

            • Not check names at all.
            • Check, but dispay only as a warning. (as your suggest)
            • Check and display as a error. But never forbid it actually (as current implemantation).

            Let me have time to consider which is the best way.

            Show
            ikedam ikedam added a comment - I'm not sure now. I know I can take one of follewing ways: Not check names at all. Check, but dispay only as a warning. (as your suggest) Check and display as a error. But never forbid it actually (as current implemantation). Let me have time to consider which is the best way.
            Hide
            ikedam ikedam added a comment -

            https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/Util.java#L116

            /**
             * Pattern for capturing variables. Either $xyz, ${xyz} or ${a.b} but not $a.b, while ignoring "$$"
             */
            private static final Pattern VARIABLE = Pattern.compile("\\$([A-Za-z0-9_]+|\\{[A-Za-z0-9_.]+\\}|\\$)");
            

            This seems available from jenkins-1.526 (JENKINS-16660). Dots are not accepted before.

            Show
            ikedam ikedam added a comment - https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/Util.java#L116 /** * Pattern for capturing variables. Either $xyz, ${xyz} or ${a.b} but not $a.b, while ignoring "$$" */ private static final Pattern VARIABLE = Pattern.compile( "\\$([A-Za-z0-9_]+|\\{[A-Za-z0-9_.]+\\}|\\$)" ); This seems available from jenkins-1.526 ( JENKINS-16660 ). Dots are not accepted before.
            Hide
            ikedam ikedam added a comment -

            I decided how to do.
            Target version is 1.3.0.

            Show
            ikedam ikedam added a comment - I decided how to do. Target version is 1.3.0.
            Show
            ikedam ikedam added a comment - Fixed as https://github.com/jenkinsci/extensible-choice-parameter-plugin/pull/7
            Hide
            ikedam ikedam added a comment -

            Really sorry for having you waiting for so long time.
            I released extensible-choice-parameter-1.3.0 including this fix.
            It will be available in a day.

            Show
            ikedam ikedam added a comment - Really sorry for having you waiting for so long time. I released extensible-choice-parameter-1.3.0 including this fix. It will be available in a day.
            Hide
            zioschild Sven Appenrodt added a comment -

            Works perfect. Thanks

            Show
            zioschild Sven Appenrodt added a comment - Works perfect. Thanks

              People

              • Assignee:
                ikedam ikedam
                Reporter:
                zioschild Sven Appenrodt
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: