Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-26135] Allow CPS global lib scripts to define global variables #131

Merged
merged 47 commits into from Aug 20, 2015

Conversation

Projects
None yet
7 participants
@kohsuke
Copy link
Member

commented May 12, 2015

This allows the release engineering team to define higher-level organization specific workflow DSLs.

This is critical to use workflow as "simplified text-based UX". Such DSLs could be for example:

acmeCorp {
    id = '123'
    devEnvironment = 'qa-svr-1'
}

(where let's say there's an inventory of application ID in the central database describing where the repository is, and 'qa-svr-1' refers to the Tomcat server where the app gets deployed at the end.)

  • PoC
  • Define top-level vars dir and pick scripts from there, not from src. (perhaps)
  • Tweak GlobalVariable API to get rid of dynamic extension list manipulation.
  • Doc updates.
  • Fall back to plain text help
  • Solve JENKINS-26135
  • Use JENKINS-28586 fix from jenkinsci/script-security-plugin#13
  • Deal with regressions from JENKINS-28586; e.g. LoadStepTest.basics failure

@reviewbybees

Allow CPS global lib scripts to define global variables.
This allows the release engineering team to define higher-level
organization specific workflow DSLs.

This is critical to use workflow as "simplified text-based UX".
Such DSLs could be for example:

acmeCorp {
    id = '123'
    devEnvironment = 'qa-svr-1'
}

(where let's say there's an inventory of application ID in the
central database describing where the repository is, and 'qa-svr-1'
refers to the Tomcat server where the app gets deployed at the end.)
@kohsuke

This comment has been minimized.

Copy link
Member Author

commented May 12, 2015

I could have avoided registering/unregistering a new Extension on the fly if GlobalVariable supports a collection of global variables, not just one.

It looks like GlobalVariable isn't released yet, so maybe you are open to changing it.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented May 12, 2015

  • I'm aware that UserDefinedGlobalVariableList takes all scripts in ${Workspace}/src folder. It means that you cannot place any other Groovy script there, so it may cause a conflict. Probably you could get scripts from somewhere like ${Workspace}/src/globalVars/ or use a predefined wildcard
  • One script can contribute only one variable. I suppose it's not user-friendly in the use-case of "I retrieve an bulk info from a remote component and want to save it to 25 variables" (e.g. SCM stats, Docker server info, etc.)
@jglick

This comment has been minimized.

Copy link
Member

commented May 12, 2015

[INFO] Possible null pointer dereference in new org.jenkinsci.plugins.workflow.cps.global.UserDefinedGlobalVariableList() due to return value of called method [org.jenkinsci.plugins.workflow.cps.global.UserDefinedGlobalVariableList, org.jenkinsci.plugins.workflow.cps.global.UserDefinedGlobalVariableList] Dereferenced at UserDefinedGlobalVariableList.java:[line 29]Known null at UserDefinedGlobalVariableList.java:[line 29]
[INFO] Possible null pointer dereference in org.jenkinsci.plugins.workflow.cps.global.UserDefinedGlobalVariableList.init() due to return value of called method [org.jenkinsci.plugins.workflow.cps.global.UserDefinedGlobalVariableList, org.jenkinsci.plugins.workflow.cps.global.UserDefinedGlobalVariableList] Dereferenced at UserDefinedGlobalVariableList.java:[line 69]Known null at UserDefinedGlobalVariableList.java:[line 69]
@jglick

This comment has been minimized.

Copy link
Member

commented May 12, 2015

It looks like GlobalVariable isn't released yet

Well, it is in 1.7-alpha-1, but I think it is fine to change its signature if that makes this easier.

* @author Kohsuke Kawaguchi
*/
// not @Extension because these are manually registered
public class UserDefinedGlobalVariable extends GlobalVariable {

This comment has been minimized.

Copy link
@jglick

jglick May 12, 2015

Member

Need not be public I guess.

@jglick

This comment has been minimized.

Copy link
Member

commented May 12, 2015

As with @oleg-nenashev I am a little concerned about overloading $JENKINS_HOME/workflow-libs/src/, which is already used for classes you intend to import, in a package structure. Would seem safer to use a separate subdirectory for this RFE that emphasizes that it should contain files in a flat structure (no packages) and that the file name becomes a variable. IIRC the reason we decided to use the src/ subdirectory in 1.0 for this plugin was exactly so that we could compatibly introduce other subdirectories for different purposes.

@jglick

This comment has been minimized.

Copy link
Member

commented May 12, 2015

BTW you should add a CHANGES.md entry.

@@ -0,0 +1,4 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core">
<j:out value="${it.helpHtml}"/>

This comment has been minimized.

Copy link
@jglick

jglick May 12, 2015

Member

Careful—this is user content emitted raw. It would be safer to use MarkupFormatter, or restrict to plain text. Admittedly it currently requires RUN_SCRIPTS to push to this repo, but see JENKINS-26538.

This comment has been minimized.

Copy link
@kohsuke

kohsuke May 12, 2015

Author Member

All right, I'll do MarkupFormatter and probably name those files as just *.txt.

This comment has been minimized.

Copy link
@jglick

jglick Aug 19, 2015

Member

(resolved)

*/
@Initializer(fatal=false,after= InitMilestone.EXTENSIONS_AUGMENTED,before=InitMilestone.JOB_LOADED)
public static void init() {
Jenkins.getInstance().getExtensionList(UserDefinedGlobalVariableList.class).get(UserDefinedGlobalVariableList.class).rebuild();

This comment has been minimized.

Copy link
@jglick

jglick May 12, 2015

Member

Definitely this is ugly and would benefit from a change to GlobalVariable to return a Set<String> names and take a String name parameter to getValue.

(Could even do so compatibly by introducing a GlobalVariables extension point with that signature, then retrofitting GlobalVariable to implement it with final methods delegating to its current signature, though this seems like overkill for an API introduced only in an alpha.)

@jglick

This comment has been minimized.

Copy link
Member

commented May 12, 2015

I retrieve an bulk info from a remote component and want to save it to 25 variables

I guess this use case, if it ever comes up, could be treated as an additional feature—so long as we use distinctive subdirectory names.

Other than the comments above this looks good to me.

BTW have you checked that this functionality is not already possible without any change? Since src/ is already in the script “classpath”, it seems that Groovy might have interpreted references to someVar.someMethod as static calls to a class in the default package. But maybe you then run aground trying to access an implicit CpsScript (for steps or other global variables) from a static context.

@jglick

This comment has been minimized.

Copy link
Member

commented May 12, 2015

As an aside, the consistent feedback I hear from everyone I talk to about Workflow scripts is that they want to keep all sources in their own Git repositories (or other SCM), and thus would not use this plugin. Not blocking this PR but we should be considering how similar features could be built using external SCMs.

@kohsuke

This comment has been minimized.

Copy link
Member Author

commented May 12, 2015

  • I thought about possibility of name conflicts, and putting those global variables into a fixed package or a separate directory (like ${globalLibs}/vars or something like that.) But a full day conversations with users the other day made me feel that this is probably the most common use case for CPS global libs that I will be pushing to users, so occupying the root package felt justifiable. It also felt natural; if you write org.acme.foo then it maps to src/org/acme/foo.groovy, so if it's just acme, one would expect that it would map to src/foo.groovy. I'll wait for others to chime in.
  • GlobalVariable is unsuitable for dynamically listed variables like that, but I don't think that's an issue --- certainly not a problem in this PR. It's not really meant to be used for simple values, it's meant to be used more like a namespace.
@jglick

This comment has been minimized.

Copy link
Member

commented May 12, 2015

It also felt natural; if you write org.acme.foo then it maps to src/org/acme/foo.groovy, so if it's just acme, one would expect that it would map to src/foo.groovy.

Yet these behave in subtly different ways. One creates a class definition, one a variable. Just worried that this is adding yet another mine to the field; see the explanation of explicit vs. implicit classes in cps-global-lib/README.md. (BTW this PR should include a patch to that page to document the new feature.)

Do not feel strongly about it, but I still find it clearer to use a separate directory namespace for a distinct feature.

It's not really meant to be used for simple values, it's meant to be used more like a namespace.

Not sure I followed that. Anyway this PR should do something to clean up the interaction with GlobalVariable—whether changing its API, or introducing a new “friend API” in workflow-cps that GlobalVariable would implement as a special case. Dynamic modifications to ExtensionList (and new @Initializers) are never acceptable unless there is no compatible alternative.

@kohsuke

This comment has been minimized.

Copy link
Member Author

commented May 12, 2015

they want to keep all sources in their own Git repositories (or other SCM)

People using another Git repository as the source of truth is anticipated and well supported. They just need to "deploy" the changes by pushing it into this repo, which is the same thing Heroku and others do.

Using other SCMs would indeed go beyond the capability of this repository. Maybe we can split cps-globa-libs into the part that just assumes populated $JENKINS_HOME/workflowLibs and another that makes that a Git repository. In that way we leave options open for a future expansion that does SCM.checkout().

@jglick

This comment has been minimized.

Copy link
Member

commented May 12, 2015

In that way we leave options open for a future expansion that does SCM.checkout().

Better to do nothing until we implement that future expansion and see exactly what it requires. I suspect that it should work like, or even as a part of, CpsScmScriptDefinition: when the build starts, multiple repos are checked out and added to the “classpath”.

@jglick

This comment has been minimized.

Copy link
Member

commented May 13, 2015

Are you still working on this?

@kohsuke

This comment has been minimized.

Copy link
Member Author

commented May 14, 2015

For the past two days I've been away. My understanding based on the conversation here is that the following tasks remain:

  • Define top-level vars dir and pick scripts from there, not from src.
  • Tweak GlobalVariable API to get rid of dynamic extension list manipulation.
  • Doc updates.
@jglick

This comment has been minimized.

Copy link
Member

commented May 14, 2015

Define top-level vars dir

Your judgment call; I am only -0 on the current placement.

@jglick

This comment has been minimized.

Copy link
Member

commented May 14, 2015

BTW I took the liberty of moving your list into a GH tasklist for easier tracking.

@jglick

This comment has been minimized.

Copy link
Member

commented May 15, 2015

Just remembered JENKINS-26135, which requests automatically loaded global functions (opposed to variables) here. Seems like it would not be hard to generalize this a bit to allow you to hook into CpsScript.invokeMethod as well as .getProperty, right? Or perhaps you can just define a variable whose value is actually a Closure; Groovy would then let you call it like a function, I think (but needs to be proven in a test).

@kohsuke kohsuke changed the title Allow CPS global lib scripts to define global variables. [WiP] Allow CPS global lib scripts to define global variables. May 22, 2015

@kohsuke

This comment has been minimized.

Copy link
Member Author

commented May 22, 2015

Good point about JENKINS-26135. I think I should be able to solve it at the same time.

kohsuke added some commits May 22, 2015

Define a new extension point GlobalVariableSet to support dynamically…
… changing list of GlobalVariables.

To make the common case easy, GlobalVariable is still left as a separate extension point. To reduce the error prone-ness of listing all variables,
I added GlobalVariable.ALL.
Switch back to plain text from HTML.
... to prevent XSS vulnerability from attackers who can define scripts.
MarkupFormatterDescriptor should declare the desired extension, so that
if Jenkins is configured for a specific MarkupFormatter, such as
MarkDown, then we can support it.

Until that day, better to stick to plain text.
[JENKINS-26135]
Global variable should be usable as a function, just like in Groovy
Closure property can be called like a function.

jglick added some commits Aug 19, 2015

Make sure we are actually testing a call(body) definition as stated i…
…n the use case, not a syntactic variant.
@jglick

This comment has been minimized.

Copy link
Member

commented Aug 19, 2015

🐝

z.checkOutFrom(repo)
```

### Defining global functions

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Aug 19, 2015

Member

One of the use cases of this seem to be a friendlier syntax for workflow-compatible but not explicitly workflow-supporting plugins that would use generic steps. Maybe have a specific example of that here?

This comment has been minimized.

Copy link
@kohsuke

kohsuke Aug 19, 2015

Author Member

Hmm, for me the syntax simplification for the generic step is a problem on its own. You are right that this could be a workaround while we solve that, but not sure if we want to advertise it in README.

This comment has been minimized.

Copy link
@jglick

jglick Aug 20, 2015

Member

Right that is a separate (filed) issue, not really related to this PR.

| +- Bar.groovy # for org.foo.Bar class
+- vars
+- foo.groovy # for global 'foo' variable/function
+- foo.txt # help for 'foo' variable/function

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Aug 19, 2015

Member

Not documented sufficiently, should mention transform of configured markup formatter.

This comment has been minimized.

Copy link
@jglick

jglick Aug 20, 2015

Member

(resolved, I think)

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Aug 19, 2015

What happens when a file name such as foo.bar.groovy is used? It defines a variable that is listed but cannot actually be accessed?

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Aug 19, 2015

🐛 until the foo.bar question is answered, and the documentation for the help file is improved.

@kohsuke

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2015

foo.bar is an invalid class name, and so you will not be able to load a script from foo.bar.groovy.

@jglick

This comment has been minimized.

Copy link
Member

commented Aug 20, 2015

@kohsuke you closed the repository for com.cloudbees.groovy-cps:1.5 but you did not release it (I did so just now), so I cannot yet push the change that switches to non-SNAPSHOT dependencies. Will do that before merge, once it makes its way to Central. (BTW it would be great if the CI builder could pick up https://oss.sonatype.org/content/groups/public/ so that we do not need to wait several hours for Central propagation after releasing libraries.)

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Aug 20, 2015

foo.bar is an invalid class name, and so you will not be able to load a script from foo.bar.groovy.

@kohsuke It looks like it would show up in the list though when it should not.


The `vars` directory hosts scripts that define global variables accessible from
workflow scripts.
The basename of each `*.groovy` file should be a Groovy (~ Java) identifier, conventionally `camelCased`.

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Aug 20, 2015

Member

must, if you want it to work.

@jglick

This comment has been minimized.

Copy link
Member

commented Aug 20, 2015

It looks like it would show up in the [list?]

It would, but oh well. Just do not do that.

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Aug 20, 2015

Right. Good enough I suppose.

🐝

The `vars` directory hosts scripts that define global variables accessible from
workflow scripts.
The basename of each `*.groovy` file should be a Groovy (~ Java) identifier, conventionally `camelCased`.
The matching `*.txt`, if present, can contain documentation, processed through the system’s configured markup formatter

This comment has been minimized.

Copy link
@jtnord

jtnord Aug 20, 2015

Member

This seems bad - as this can be changed independently and then this would affect the help rendering here.
I guess no different to all the job descriptions though.

This comment has been minimized.

Copy link
@jglick

jglick Aug 20, 2015

Member

Yes, the same problem applies to any saved markup I am afraid.

The forced *.txt extension is ugly but MarkupFormatterDescriptor lacks an API for specifying a conventional file extension.

about the block syntax in Groovy.

### Defining global variables
Internally, scripts in the `vars` directory are instantiated as a singleton on-demand, when used first.

This comment has been minimized.

Copy link
@jtnord

jtnord Aug 20, 2015

Member

OT: is there an equivalent for files not stored in the cps-global-lib?

This comment has been minimized.

Copy link
@jglick

jglick Aug 20, 2015

Member

No, and to my mind that is a crucial drawback (@kohsuke seems to think it is a minor enhancement). Requires some design thought. Probably a lot of the infrastructure work done here could be reused.

This comment has been minimized.

Copy link
@jtnord

jtnord Aug 20, 2015

Member

(OT:) at scale (and even not at scale) you likely want code review (e.g. Gerrit / GitHub PR) and may have multiple groups using a single Jenkins instance - so a global cps-global-lib becomes less useful and the desire to store these elsewhere becomes greater.

This comment has been minimized.

Copy link
@jglick

jglick Aug 20, 2015

Member

Exactly my argument. You can keep your libraries in a proper SCM, and just “deploy” them as needed to Jenkins using git push (possibly after exporting/importing from another SCM). Awkward.

Crucially (to me; @kohsuke thinks it is minor), there is no way of specifying a library version from a single project’s Jenkinsfile. It is like having a pom.xml where every plugin is using <version>LATEST</version> and the build could start failing without warning.

The groovy source files in these directories get the same sandbox / CPS
transformation just like your workflow scripts.

Other directories under the root are reserved for future enhancements.

This comment has been minimized.

Copy link
@tfennelly

tfennelly Aug 20, 2015

Member

Not part of this PR, but I think the docs for this overall feature should clarify what is meant by "a 'shared library script' Git repository inside Jenkins". Specifically, I have no idea what it means when saying "inside Jenkins".

I'm also wondering why a src and a vars folder is needed in the repo i.e. why doesn't the src folder store everything?

This comment has been minimized.

Copy link
@tfennelly

tfennelly Aug 20, 2015

Member

Okay, I see later how it works (WorkflowLibRepository, FileBackedHttpGitRepository etc).

This comment has been minimized.

Copy link
@jglick

jglick Aug 20, 2015

Member

I have no idea what it means when saying "inside Jenkins".

Does the Accessing repository section below clear it up? Probably we need some docs reorganization (CC @estheryouhana).

why doesn't the src folder store everything?

Because “variables” are accessed in a different way than importable classes with a package structure.

@jtnord

This comment has been minimized.

Copy link
Member

commented Aug 20, 2015

🐝 couldn't see anything obviously wrong.

File help = source(".txt");
if (!help.exists()) return null;

return Jenkins.getActiveInstance().getMarkupFormatter().translate(FileUtils.readFileToString(help, Charsets.UTF_8));

This comment has been minimized.

Copy link
@tfennelly

tfennelly Aug 20, 2015

Member

I assume there's a default Markup Formatter ?

This comment has been minimized.

Copy link
@jglick

jglick Aug 20, 2015

Member

Yes, it is @Nonnull.

if (o == null || getClass() != o.getClass()) return false;

UserDefinedGlobalVariable that = (UserDefinedGlobalVariable) o;
return name.equals(that.name);

This comment has been minimized.

Copy link
@tfennelly

tfennelly Aug 20, 2015

Member

Looks like name can be null?

This comment has been minimized.

Copy link
@jglick

jglick Aug 20, 2015

Member

I hope not. The getter is @Nonnull at least. The constructor is called with FilenameUtils.getBaseName, which should return nonnull when passed nonnull, which we do (File.getName).

@tfennelly

This comment has been minimized.

Copy link
Member

commented Aug 20, 2015

🐝

Insofar as I get it. Only potential problem I saw was the potential NPE in UserDefinedGlobalVariable, which I'm betting is not really possible anyway.

jglick added a commit that referenced this pull request Aug 20, 2015

Merge pull request #131 from jenkinsci/user-defined-global-libs
[JENKINS-26135] Allow CPS global lib scripts to define global variables

@jglick jglick merged commit 28c9600 into master Aug 20, 2015

1 check passed

Jenkins This pull request looks good
Details

@jglick jglick deleted the user-defined-global-libs branch Aug 20, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.