Clarify what global variables really are and how they behave in Shared Libraries #494

rtyler · 2016-12-08T19:13:46Z

This should help explain behavior seen and reported in tickets like JENKINS-39565

rtyler · 2016-12-08T19:20:20Z

@jenkins-infra/copy-editors if you have some time

omehegan · 2016-12-08T19:34:51Z

content/doc/book/pipeline/shared-libraries.adoc

+=== Defining global variables
+
+Internally, scripts in the `vars` directory are instantiated as a singleton
+on-demand, when used first.  So it is possible to define more methods,


"So it is possible" reads like a sentence fragment. Should it be something like "more methods or properties?"

s/instantiated as a singleton on-demand, when used first./instantiated as a singleton. They created on-demand the first time they are called./

s/So it is possible/This makes it possible/

omehegan · 2016-12-08T19:37:12Z

content/doc/book/pipeline/shared-libraries.adoc

-should be created and have a `call` method defined:
+For example, to define `helloWorld`, the file `vars/helloWorld.groovy`
+should be created and should implement a `call` method. The `call` method
+allows the global variable to be invoked in a manner similar to an step:


"similar to a step"

"allows a global"

omehegan · 2016-12-08T19:42:12Z

Done with review.

daniel-beck

Minor issues/possible clarifications, but the methodality needs to be fixed at least.

daniel-beck · 2016-12-08T22:12:28Z

content/doc/book/pipeline/shared-libraries.adoc

@@ -89,7 +89,7 @@ the use-case. _Manage Jenkins » Configure System » Global Pipeline Libraries_
 as many libraries as necessary can be configured.

 Since these libraries will be globally usable, any Pipeline in the system can
-utilize functionality implemented in these libraries.
+utilize methodality implemented in these libraries.


Pretty sure this isn't a word.

hah, missed this in my manual diff after search/replace

daniel-beck · 2016-12-08T22:13:58Z

content/doc/book/pipeline/shared-libraries.adoc

+=== Defining global variables
+
+Internally, scripts in the `vars` directory are instantiated as a singleton
+on-demand, when used first.  So it is possible to define more methods,


Weird comma placement between methods and properties.

daniel-beck · 2016-12-08T22:15:04Z

content/doc/book/pipeline/shared-libraries.adoc

+----
+// vars/acme.groovy
+def setFoo(v) {
+    this.foo = v;


There's a weird behavior in Pipeline scripts when variables/fields are the same name as the getter (resulting in stack overflows) – does this not apply when using this, or when in a global library?

Verified that this does not result in stack overflows

@daniel-beck http://docs.groovy-lang.org/latest/html/documentation/#properties - see the third example.
"this.name will directly access the field because the property is accessed from within the class that defines it"

@rtyler Thanks.

@bitwiseman Could be specific to scripts then, I saw some weird behavior in Pipeline that was different in standalone Groovy. Will need to look that up though.

@daniel-beck - for scripts, I totally believe you'll get an overflow, specifically you're not "in a class".

daniel-beck · 2016-12-08T22:15:33Z

content/doc/book/pipeline/shared-libraries.adoc

+}
+----
+
+Then the Pipeline Script can invoke these methods, rooted off the `acme`


"rooted off"?

Agreed, "rooted off" is a weird verbing here

daniel-beck · 2016-12-08T22:16:05Z

content/doc/book/pipeline/shared-libraries.adoc


+Shared Libraries can also define global variables which look and feel like


look and feel -> behave?

daniel-beck · 2016-12-08T22:16:52Z

content/doc/book/pipeline/shared-libraries.adoc


-For example, to define `helloWorld` step, the file `vars/helloWorld.groovy`
-should be created and have a `call` method defined:
+For example, to define `helloWorld`, the file `vars/helloWorld.groovy`


IIRC they need to be camelCased rather than PascalCased – confirm, and if so, add a NOTE.

I confirmed this, WEIRD! I didn't know that was an actual requirement. PascalCased results in the script not even compiling!

org.codehaus.groovy.control.MultipleCompilationErrorsException: startup failed: WorkflowScript: 2: unexpected token: Mr Jenkins @ line 2, column 14. SayHello "Mr Jenkins"

daniel-beck · 2016-12-08T22:17:38Z

content/doc/book/pipeline/shared-libraries.adoc


 [source,groovy]
 ----
 // vars/helloWorld.groovy
 def call(name) {
-    // you can call any valid step functions from your code, just like you can from Pipeline scripts
+    // Any valid steps can be called from this code, just like in other
+    // Pipeline scripts


Ambiguous wording given @NonCPS not being able to do that despite being in a Pipeline script.

I'm not sure I understand the edge case you're describing as far as the @NonCPS annotation goes, can you explain a bit more or link to something describing the issue more?

You're saying that steps can be called from here, "just like in other Pipeline scripts", however only the non-@NonCPS parts of a Pipeline script can call steps.

Since this document doesn't introduce the @NonCPS annotation in any shape or form, I do not think this source snippet is the appropriate place to do so.

IMO we should find a way to introduce @NonCPS in this section, in some form other than the most fitting title of "Crazy wacky shit you might run into"

@daniel-beck Turns out somebody filed a ticket, WEBSITE-252, which captures the desire to have stuff like @NonCPS documented.

I have assigned that to myself

daniel-beck · 2016-12-08T22:20:00Z

content/doc/book/pipeline/shared-libraries.adoc

+====
+The `call()` method will not be automatically invoked if `helloWorld` is
+referenced without *any* arguments. Implementing a global variable which
+requires no default arguments **must** be explicitly invoked like a method, for


AFAIU this is the same as with any Groovy methods (even though this isn't a method), no? Maybe rephrase if that's the case?

I'm not sure I understand what you're asking here. It would be the same with all Groovy objects. The global variable helloWorld is just an object stuck into the scope of the Pipeline Script, and just referring to it like helloWorld is just referencing a variable like any other

I don't think (AFAIK) any normal methods can be called as foo either in Groovy. It's always foo(), or foo "arg". If so, this explanation basically states "It's Groovy syntax, deal with it".

Considering that I have hit this behavior, and at least one other person hit it hard enough to file a JIRA about it, I think it's worth keeping a note in the documentation

Right, but the documentation reads like this is something very special and limited to steps, when it's simply not (AFAIK). Any parameterless method call needs parentheses (again, AFAIK).

Replace the example with:

def call(String name = 'Human') { // Any valid steps can be called from this code, just like in other // Pipeline scripts echo "Hello, ${name}." }

Calling example:

[source,groovy] ---- greetings 'Joe' greetings() ----

This shows what you're noting with reiterating basic groovy language facts.

I think the updated example you suggest @bitwiseman, with an inline code comment perhaps, does a decent job conveying things here.

I quite dislike the tone I'm perceiving in this review here though. There was a stumbling block which I hit (relatively savvy Pipeline user) and at least other user hit, hard enough to file a ticket.

Avoiding referencing or spelling out how "basic groovy language facts" work in Pipeline is a slightly more dressed up version of "lrn2groovy noob" in my opinion.

If users are hitting stumbling blocks, or having problems with Pipeline Script, that is a bug to me. Whether it's fixed in the UI, documentation, or Pipeline's syntax, something should be done to help future users avoid the same potholes.

That said, @bitwiseman are you planning on submitting a subsequent PR, or should I incorporate your updated examples here?

@rtyler -
Since you agree it serves the purpose. Please incorporate.

If users are hitting stumbling blocks, or having problems with Pipeline Script, that is a bug to me. Whether it's fixed in the UI, documentation, or Pipeline's syntax, something should be done to help future users avoid the same potholes.

I totally agree with you and I apologize for any tone issue.

The discussion (and subsequent style guideline) that needs to happen is "How do we give people the notes they need without repeatedly reminding readers about Groovy behavior?"

Maybe having an appendix in the handbook that talks about Groovy pitfalls?

A primer on Pipeline Groovy (Groovy but limited to what actually works and pointing out what fancy Groovy features don't work) would be an amazing reference for anyone trying to graduate past the "pure configuration DSL" kind of Pipeline scripts (loops, conditions, …) and people already familiar with Groovy and/or Java to get an idea what works and what doesn't to reduce trial/error. This way, people don't need to go through the progression from "Oh, coll.each { } is broken", over "Oh, for (foo in coll) is broken" to C-style loops.

This would also have the advantage of reducing the number of notes/warnings needed in all other parts of the Pipeline documentation.

Assuming this doesn't exist yet, of course. Haven't read all of the new Pipeline docs.

daniel-beck · 2016-12-08T22:22:34Z

content/doc/book/pipeline/shared-libraries.adoc


 [source,groovy]
 ----
-// vars/jenkinsPlugin.groovy
+// vars/buildPlugin.groovy


Why remove the jenkins? Wouldn't that make the example clearer wrt what is being built here?

I wanted to change this because I can link to the actual implementation of a global variable named buildPlugin once it's better documented and better code

rtyler · 2016-12-09T00:20:39Z

@daniel-beck Updated the latest commit with fixes.

bitwiseman · 2016-12-09T00:12:44Z

content/doc/book/pipeline/shared-libraries.adoc

+
+Internally, scripts in the `vars` directory are instantiated as a singleton
+on-demand, when used first.  So it is possible to define more methods,
+properties on a single file that interact with each other:


s/ methods, properties on / methods and properties in /

bitwiseman · 2016-12-09T00:22:40Z

content/doc/book/pipeline/shared-libraries.adoc

+----
+// vars/acme.groovy
+def setFoo(v) {
+    this.foo = v;


@daniel-beck http://docs.groovy-lang.org/latest/html/documentation/#properties - see the third example.
"this.name will directly access the field because the property is accessed from within the class that defines it"

bitwiseman · 2016-12-09T00:25:16Z

content/doc/book/pipeline/shared-libraries.adoc

+def say(name) {
+    echo "Hello world, ${name}"
+}
+----


I don't love this example. The text above talks about methods and properties in "a single file that interact with each other", but this example only shows the property getter and setter interacting, not the other method.

Then you can provide a new example in a pull request that you're more happy with 😈

Sure, it'll look like this:

// vars/acme.groovy def setName(value) { this.name = value; } def getName() { return this.name; } def caution(message) { echo "Hello, ${this.name}! CAUTION: ${message}" }

Example:

acme.name = "Alice"; echo acme.name; // print "Alice" acme.caution "The queen is angry!" // print "Hello, Alice. CAUTION: The queen is angry!"

bitwiseman · 2016-12-09T00:31:27Z

content/doc/book/pipeline/shared-libraries.adoc

+====
+A variable defined in a shared library will only show up in _Global Variables
+Reference_ (under _Pipeline Syntax_) after you have first run a successful
+build using that library, allowing its sources to be checked out by Jenkins.


s/ you have first run a successful build using that library, allowing its sources to be checked out by Jenkins. / Jenkins loads and uses that library as part of a successful build. /

bitwiseman · 2016-12-09T00:33:11Z

content/doc/book/pipeline/shared-libraries.adoc

-should be created and have a `call` method defined:
+For example, to define `helloWorld`, the file `vars/helloWorld.groovy`
+should be created and should implement a `call` method. The `call` method
+allows the global variable to be invoked in a manner similar to an step:


"allows a global"

bitwiseman · 2016-12-09T00:42:09Z

content/doc/book/pipeline/shared-libraries.adoc

+====
+The `call()` method will not be automatically invoked if `helloWorld` is
+referenced without *any* arguments. Implementing a global variable which
+requires no default arguments **must** be explicitly invoked like a method, for


"A call method may be defined with no arguments - def call() { ... }. However, as with any Groovy method, when we invoke a method without arguments it must have empty parentheses after it. For example, helloWorld() instead of helloWorld."

…ies are just variables Basically, the end-user can pretend that they're just like other "steps" but in truth we're using some Groovy invocation magic which means parameterless invocation doesn't work in the same manner as a real step. Fixes JENKINS-39565

Instead of using metohds/functions interchangeably. This also removes any references to "global methods" and instead refers to those entities as "global variables" to ensure we avoid confusion around what those /things/ actually are.

bitwiseman · 2016-12-09T22:31:36Z

🐝

omehegan reviewed Dec 8, 2016

View reviewed changes

daniel-beck requested changes Dec 8, 2016

View reviewed changes

rtyler force-pushed the shared-library-clarifications branch from ec88a88 to 9fc9df0 Compare December 9, 2016 00:20

bitwiseman suggested changes Dec 9, 2016

View reviewed changes

R. Tyler Croy added 2 commits December 9, 2016 14:09

rtyler force-pushed the shared-library-clarifications branch from 9fc9df0 to 290d581 Compare December 9, 2016 22:21

daniel-beck approved these changes Dec 9, 2016

View reviewed changes

rtyler merged commit 45de616 into jenkins-infra:master Dec 9, 2016

rtyler deleted the shared-library-clarifications branch December 9, 2016 22:32


		Shared Libraries can also define global variables which look and feel like

Clarify what global variables really are and how they behave in Shared Libraries #494

Clarify what global variables really are and how they behave in Shared Libraries #494

Conversation

rtyler commented Dec 8, 2016

rtyler commented Dec 8, 2016

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

omehegan commented Dec 8, 2016

daniel-beck 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

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

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

Choose a reason for hiding this comment

daniel-beck Dec 9, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtyler commented Dec 9, 2016

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

bitwiseman Dec 9, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitwiseman Dec 9, 2016 • edited

Choose a reason for hiding this comment

bitwiseman commented Dec 9, 2016

daniel-beck Dec 9, 2016 •

edited

bitwiseman Dec 9, 2016 •

edited

bitwiseman Dec 9, 2016 •

edited