[JENKINS-30222] Multibranch/global var incompatibility

[jglick]

@kohsuke FYI

[jglick]

@@ -208,13 +206,13 @@ public class SCMBinderTest {
                     "  body.delegate = config\n" +
                     "  body()\n" +
                     "  node {\n" +
-                    "    checkout scm\n" +
+                    "    checkout config.scm\n" +
                     "    echo \"loaded ${readFile config.file}\"\n" +
                     "  }\n" +
                     "}\n");
                 // Then a project using it:
                 sampleGitRepo.init();
-                sampleGitRepo.write("Jenkinsfile", "standardJob {file = 'resource'}");
+                sampleGitRepo.write("Jenkinsfile", "def _scm = scm; standardJob {scm = _scm; file = 'resource'}");
                 sampleGitRepo.write("resource", "resource content");
                 sampleGitRepo.git("add", "Jenkinsfile");
                 sampleGitRepo.git("add", "resource");

works but the Jenkinsfile side is too ugly to take seriously.

[jglick]

This also works, and has the benefit of making the library code comprehensible to mere mortals, though it is still a little cumbersome on the Jenkinsfile side, and looks a little less like a configuration file (the original goal):

@@ -202,19 +200,15 @@ public class SCMBinderTest {
                 File vars = new File(repo.workspace, /*UserDefinedGlobalVariable.PREFIX*/ "vars");
                 vars.mkdirs();
                 FileUtils.writeStringToFile(new File(vars, "standardJob.groovy"),
-                    "def call(body) {\n" +
-                    "  def config = [:]\n" +
-                    "  body.resolveStrategy = Closure.DELEGATE_FIRST\n" +
-                    "  body.delegate = config\n" +
-                    "  body()\n" +
+                    "def call(config) {\n" +
                     "  node {\n" +
-                    "    checkout scm\n" +
+                    "    checkout config.scm\n" +
                     "    echo \"loaded ${readFile config.file}\"\n" +
                     "  }\n" +
                     "}\n");
                 // Then a project using it:
                 sampleGitRepo.init();
-                sampleGitRepo.write("Jenkinsfile", "standardJob {file = 'resource'}");
+                sampleGitRepo.write("Jenkinsfile", "standardJob scm: scm, file: 'resource'");
                 sampleGitRepo.write("resource", "resource content");
                 sampleGitRepo.git("add", "Jenkinsfile");
                 sampleGitRepo.git("add", "resource");

[jglick]

Working on using GlobalVariable rather than GroovyShellDecorator, but have to figure out how to store the value in the program when the build starts.

[jglick]

Nothing I try with GlobalVariable works. The problem is that bindings in the GroovyShell are not visible from within the library. Steps work only because each CpsScript instance (there is a new one for the library) redefines steps to a new DSL.

DockerDSL manages to use GlobalVariable, but it simply loads a new Docker object for each CpsScript which requests it. That is not an option here: I need to use the SCM which is available only when the build starts, and I can find no place to “stash” it in the program state.

Suppose a build used a library and tried to access scm after a restart. Where would it get it from? The CpsScript of the library has no apparent backreference to the original CpsScript or its GroovyShell bindings; all it has is a CpsFlowExecution and thus a WorkflowRun. I could store the SCM in an Action on the build but that adds even more redundancy stuff to an already cluttered build.xml.

[jglick]

The best I could manage so far. @reviewbybees esp. @kohsuke

[ghost]

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

[oleg-nenashev]

If I understand correctly, this test demonstrates a problem and provides a workaround, which is not very graceful. Is is correct?

🐝 if yes

[tfennelly]

The solution, as I understand it, does seem too funky.

From what I read here, seems to me like the best solution is to use an Action. I hear what you are saying about that adding more crap persisted in the build.xml, but that seems like another problem that should be solved.

Actions have always seemed cumbersome to me as a way of storing state. It seems to me like there's a need for a general purpose context storage scoped at different levels (Job, Run etc) and with different persistence specs. That's for another discussion though.

[jglick]

If I understand correctly, this test demonstrates a problem and provides a workaround, which is not very graceful. Is is correct?

Correct. cdfee0a (the first commit) demonstrates what I expected to work at the start. Basically remove the body.owner. prefix when accessing scm.

My current thinking is that the messy list of scopes could be cleaned up (and this problem solved) if we created a special Binding which was attached to the CpsThreadGroup: the program as a whole. Each CpsScript, whether the “main” script or some other file that got loaded, would delegate to this overall binding. That would hold

• steps (a DSL, which raw function names delegate to)
• GlobalVariables:
  • the standard currentBuild and env
  • also docker, from docker-workflow
  • and in this case I could add scm
• parameter values (though I would like to deprecate these and replace with a global variable like params)

[amuniz]

🐝

[jglick]

Filed JENKINS-30222 to track improvements.