[FIXED JENKINS-42498] Stop evaluating when conditions at runtime

[abayer]

• JENKINS issue(s):
  • JENKINS-42498
• Description:
  • Pull the when conditions in by translating them from the AST model, which does require some evaluation of strings here and there, but avoids having any Closures in the Describables. Note that this is on top of [FIXED JENKINS-41185] Add support for and/not/or of when conditions #125 - relevant commits start at 76a4350. If this PR looks more appropriate than [FIXED JENKINS-42498] Dehydrate GStrings in the runtime model #131, I'll close [FIXED JENKINS-42498] Dehydrate GStrings in the runtime model #131.
• Documentation changes:
  • n/a
• Users/aliases to notify:
  • @reviewbybees
  • @rsandell
  • @jglick

[abayer]

Making this a List to check against since we're doing this again for Environment over in #110, and I imagine there'll be more in the future as I gradually kill off the translators.

[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.

[jglick]

Tried to load the effective diff but it is showing commits from #125, which it should not have been; any idea?

[jglick]

Maybe just get upstream reviewed & merged, then git merge master here and GitHub will show the incremental diff automatically.

[abayer]

@jglick effective diff fixed now, but yeah, I want to get #125 merged. I would appreciate your thoughts in the meantime, though.

[jglick]

I am not sure I really understand what it is I am looking at, but it seems that for something like

environment {
  TAG = "$JOB_NAME.$BUILD_NUMBER"
}

previously you would have been passing around a Closure and calling it, and now you are passing around a String and evaluateing it?

In an actual programming language this would be a huge red flag—you are throwing away proper handling of lexical scope, and generally setting the clock back to 1961. But in Declarative I guess it is fine because by the restrictive syntactic rules imposed by the verifier you would not be able to use anything bound into a closure anyway.

[abayer]

@jglick Yeah, it's not my first choice, but in this particular case, either we're storing a Closure in a Describable and hitting the serialization problem, or we're storing a String and evaluateing it. I think the tradeoffs come down in favor of the String/evaluate option, but I definitely want your input.

[jglick]

Do not really understand the details, but sounds right in a general way.

[jglick]

Uh, does this not need to do some kind of escaping?

[abayer]

Hrm. Honestly not sure. Lemme think on it.

[abayer]

So actually, I don't think it does. We're just taking the string from the original Jenkinsfile source, in effect.