-
Notifications
You must be signed in to change notification settings - Fork 31
Hacktoberfest [JENKINS-69731] Enhance @Library annotation to load same version (feature branch name) of library as pipeline script from SCM #19
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
base: master
Are you sure you want to change the base?
Changes from 41 commits
a937477
15a252a
bf228e4
61db728
2e3c855
0f89b22
cf3d140
eb73ce4
3ef66ae
7ddf8e6
4f792f6
9d95881
0694dd1
824a204
553aba3
bcba445
6b5255c
a661a13
9700565
30d21b9
34718a4
8e92f64
5dc85dd
a9aa8f1
418c572
54bb3e9
d47dad5
811fb82
43d45b8
38f380f
68902fd
dddd3ac
48ad0fe
eeba94c
ee36d66
83cea07
cf2c069
6d74aea
6097f6a
85e96e3
4ce1164
ce8b820
2d4bd48
6767dc2
edd1ddb
5190214
06a3e09
f4c53c7
d529421
71c51c8
9e20d78
3cb60c0
ba01128
863b170
dea552f
70b029c
786092d
be0c051
d67c2d1
db74431
812d12d
6ab574a
2d098a5
6379716
ac9f40a
b95c46a
a943713
d039e50
48548ae
5ab9713
65a7d02
c2cf4b5
6668195
c1082b9
02cf65b
7926f8c
7329664
b8c4a88
b8601e9
2f0f92d
e03b265
74ea384
085904b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| <div> | ||
| If checked, scripts may select a custom version of the library | ||
| by appending literally <code>@${BRANCH_NAME}</code> in the | ||
| <code>@Library</code> annotation, to use same SCM branch name | ||
| of the library codebase as that of the pipeline being built.<br/> | ||
| If such branch name is not resolvable as an environment variable | ||
| or not present in library storage (SCM, release artifacts...), | ||
| it would fall back to default version you select here.<br/> | ||
| Keep in mind that while the markup for such variable version | ||
| specification is intentionally similar to what you would use | ||
| in pipeline Groovy code, for simpler use and maintenance, the | ||
|
Comment on lines
+9
to
+11
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So you could just use the
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Also, part of the equation is that at least here people part-time doing devops are not Java developers. Maybe what is used could go into libs or plugins. No idea really. Would it be easier arranged if it were spread around a dozen repos instead of one? No idea either. Their bread is elsewhere, in other languages and environments, and for versioned libs they need the path of least resistance to make things "just work", including that for qa/stage/prod environments :) Had a hard time with According to IRC discussion, not so much a fringe case - several posters of the few online and active in a day sounded enthusiastic about just this sort of use-case for libs that they failed to make use of too.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking of "marginal" use cases, some of our libraries dealing with infrastructure and CI farm management (housekeeping like freeing locked resources grabbed by devs for too long and their associated PR no longer exists) do integrate pretty close with Jenkins internals, so running them as a trusted global library is a lot easier than writing straight infra pipelines and keeping permitting methods through sandboxing protection, one run at a time. I do not know how many people really do that sort of code, going into Jenkins Items, tickling the LockableResourceManager, etc. - but since there is a ready "paste this into JENKINS_URL/script" solution for just about anything, I suppose many do. And such libs (and front-end pipelines to tap into them) are something that can in some shops benefit from evolving in sync (same branch where possible) even if spread across repos.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also thinking of the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is one way, though better I think is to have a consumer (repo with One thing I have wanted for a while is a Dependabot module for Jenkins Pipeline libraries, so you could specify an (immutable!) tag in At any rate, different installations are going to have different workflows for reasons good or bad. What I am mainly wondering is whether this new logic actually needs to be implemented here, since it is a pretty big change and maintainers of this plugin (if any; not me) can hardly keep up with basic stuff like fixes to the cache system. I suspect this could be implemented entirely as a new
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
IMO the entire notion of matching up branch names between main repo and library only makes sense if they are using the same SCM, so I would not worry about such exotic cases.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, different tech might have been too far-fetched... or not... Earlier I've worked with a project that had components built by Jenkinsfiles and most sources out on Github and built by FOSS CI farms, with a public version of JSL driving that. However the commercially supported product was built and tested by an in-house farm, using different toolkits and services, with a completely different "API-compatible" implementation of JSL served by an on-prem SCM. True, both SCMs happened to talk Git, but it was not granted (had corporate IT enable that "for simplicity" in addition to repo protocols their earlier projects used).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am afraid that Jenkins is popular enough that, statistically, any requirement will be critical for somebody! The advantage of the
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the pipeline source may not fully use classes defined by a Or is the idea to use
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jglick @dwnusbaum : regarding security... maybe I'm missing something, but I've slept on the idea and concluded that the In case of So one attack vector I'd assume here is that on SCM platform (GitHub etc.) random untrusted people are not forbidden to create branches in the library repository (originally or by attacker combined to include pipeline script) - e.g. some "main" branches are "protected" to only let maintainers update them, and other branches are available to any contributor. Can it be said that the job of security in this case is on the SCM platform (and on Jenkins side - the checkbox to Regarding a new checkbox, to allow or forbid using "this library" if pipeline seems to come from same repo, I suppose it can be done but with different ways to specify a repo URL (git-ssh, git://, https... just for git protocol alone) it can be circumvented... (or do SCM classes allow to test for "real equality" of repo URLs?)
|
||
| actual strings are expanded by the plugin as it pre-processes | ||
| the pipeline script before compilation. Tokens spelled in the | ||
| <code>@Library</code> annotation are not Groovy variables! | ||
| The values substituted here are not influenced by run-time | ||
| interpretation of the pipeline script source text! | ||
| </div> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| <div> | ||
| If checked, and if scripts are allowed to select a custom version | ||
| of the library by appending literally <code>@${BRANCH_NAME}</code> | ||
| in the <code>@Library</code> annotation, then for pull request | ||
| builds additional fall-back library branches would include the | ||
| names used as <code>CHANGE_BRANCH</code> (name of source branch | ||
| of pipeline pull request) and <code>CHANGE_TARGET</code> (name | ||
| of target branch of pipeline pull request), if such branch names | ||
| already exist in the trusted shared library repository. | ||
| </div> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| <div> | ||
| If checked, scripts may select a custom version of the library | ||
| by appending literally <code>@${env.VARNAME}</code> pattern in | ||
| the <code>@Library</code> annotation, to use current value of | ||
| chosen environment variable named <code>VARNAME</code> if it | ||
| is defined in job properties or on the build agent.<br/> | ||
| If such branch name is not resolvable as an environment variable | ||
| or not present in library storage (SCM, release artifacts...), | ||
| it would fall back to default version you select here.<br/> | ||
| Keep in mind that while the markup for such variable version | ||
| specification is intentionally similar to what you would use | ||
| in pipeline Groovy code, for simpler use and maintenance, the | ||
| actual strings are expanded by the plugin as it pre-processes | ||
| the pipeline script before compilation. Tokens spelled in the | ||
| <code>@Library</code> annotation are not Groovy variables! | ||
| The values substituted here are not influenced by run-time | ||
| interpretation of the pipeline script source text! | ||
| </div> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| <div> | ||
| If checked, the <code>defaultedVersion</code> method would print | ||
| its progress trying to resolve <code>@${BRANCH_NAME}</code> in the | ||
| <code>@Library</code> annotation into the build log. | ||
| </div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This plugin is useless for Pipeline. No idea what you are trying to do with
pipeline-groovy-lib-plugin/src/test/java/org/jenkinsci/plugins/workflow/libs/SCMSourceRetrieverTest.java
Line 1058 in 4ce1164
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Maybe you wanted a
@TestExtentionon anEnvironmentContributoror something?)Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, feared so.
As commented in the test, the idea was to see if we can have a Job or individual Run with some (externally) pre-set environment variables. I mean, any Run already does have a personal copy of EnvVars inherited from the global config or particular agent. In Git (Client?) plugin I saw similar logic to take current env, and tweak its copy before launching the external system program. Makes sense for there to be a way that somehow else we can intrude into what happens between the acts of Job definition and a particular build's start.
I hoped to test that this works, e.g. for
@Library('libname@${env.VARNAME}')the job-customized variable takes precedence - so I hopedenvinjector some other plugin can be it. If envinject is not the way (or if Pipeline generally definitely would not make it possible), that's also good to know and I can then excise this test attempt. So far I left it there for someone smarter to complete - scaffolding is ready, just some magic spell missing.For the context important here - we are not talking about DSL
environment{}orwithEnv{}settings that take place during Groovy script processing, but about environment set up for theRun<?,?>runargument ofdefaultedVersion()- before it executes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking of it, the test code schedules a build and waits for outcomes (several ways to do so). Is there a programmatic way to do what
scheduleBuilddoes but in more detailed explicit commands - e.g. to create that WorkflowRun object from a WorkflowJob, then somehow inject envvars (is the return value ofgetEnvironment()a copy of itsEnvVars, or a reference to the real list - to which we canadd()?) and just then schedule it to be built?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TestExtensionon anEnvironmentContributorcan do this. Or if you are actually testing multibranch, you can test that directly, as other tests in this plugin already do, in which case there is no need to fake anything—you can use actual Git repos with actual branches.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For kicks, also traced my implementation with envInject (the currently committed PR branch state) - it also seems to be consulted as an
Action, but fails to latch in thisEnvironmentContributingAction::buildEnvironment()line (from IDEA decompiler):From this I quickly guess envInject did not override "buildEnvVars" method and so is ignored. But otherwise
thisdoes contain myenvMapcontents and would have applied here (and probably lost toWorkflowRunoverwriting it from global config, per above.Also earlier in
getEnvironmentthe plugin was consulted forEnvInjectEnvVarsContributor::buildEnvironmentFor()but skipped because it gotnullfrom the likes of:UPDATE (if someone were to fix envinject for pipelines) Looking at actual sources:
buildEnvVarsEnvInjectPluginAction easo not sure why it gets skipped):Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed the recent changes to the test code (which so far fails, but seems the preferences of
WorkflowRundetailed above explain why). I suppose I'd break the test into pieces to try such injection without changing global config, to prove that a plugin might technically influence the choice of@Libraryvia envvars (although just now it would not be envinject, it seems). Might keep the current test code in place to confirm that global-config envvars always land on top, now that it is sort of expected. And so far there is a place-holder, but perhaps I would find a way to tap into "built-in" node to change its config and see that it has lower priority.But all that later tonight or next week, probably...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the test update regarding "injected" envvars vs. global config - PR updated.
Hope to find how to manage the "built-in" node config and add its envvars to the realistic use-cases modeled by the test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, do not call
add, use the@TestExtensionannotation as previously mentioned.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, did not wrap my head around those (too advanced Java I guess), so far posted what works for me :)
At least, the test restores the original list of contributors (to check resulting behavior in a next combo within same test method), so the addition is short-lived and does not impact other test cases (its debug trace does not pop up unexpectedly).
Updated the PR now with tests manipulating the "built-in" node's envvar configuration, with the variable NOT set in global config, although this did not work the way I expected it to:
Computer.getEnvironment()and the only way to clear the cache is to reconnect the agent (after the fix), delete and redefine it (before the fix), or restart Jenkins. Neither of these apply to "built-in" node (can't disconnect or delete) and the JenkinsRule instance respectively.cachedEnvironmentfield tonulland checked it with the debugger - it was repopulated from thegetChannel()which forMasterComputerended up reading global config.So effectively the updated end of test currently verifies just that - the expected behavior is what it is (and not so much what I naively wanted it to be); but now we would be alarmed if the core changes (fixes) it and this test can get updated accordingly.