-
Notifications
You must be signed in to change notification settings - Fork 25
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
Hacktoberfest [JENKINS-69731] Enhance @Library annotation to load same version (feature branch name) of library as pipeline script from SCM #19
Open
jimklimov
wants to merge
78
commits into
jenkinsci:master
Choose a base branch
from
jimklimov:JENKINS-69731-scm-BRANCH_NAME
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
78 commits
Select commit
Hold shift + click to select a range
a937477
LibraryConfiguration: extend with allowBRANCH_NAME for literal use of…
jimklimov 15a252a
LibraryConfigurationTest.java: add tests for LibraryConfiguration.def…
jimklimov bf228e4
SCMSourceRetrieverTest.java: add checkDefaultVersion() for interactio…
jimklimov 61db728
SCMSourceRetrieverTest: checkDefaultVersion(): also test non-checkout…
jimklimov 2e3c855
LibraryConfiguration: extend with allowBRANCH_NAME_PR to let CHANGE_B…
jimklimov 0f89b22
LibraryConfiguration: defaultedVersion(): for WorkflowRun check getSC…
jimklimov cf3d140
LibraryConfiguration: defaultedVersion(): check for BRANCH_NAME first…
jimklimov eb73ce4
SCMSourceRetrieverTest: rename checkDefaultVersion() to checkDefaultV…
jimklimov 3ef66ae
SCMSourceRetrieverTest: add checkDefaultVersion_BRANCH_NAME_MBP() [JE…
jimklimov 7ddf8e6
SCMSourceRetrieverTest.java: add checkDefaultVersion_MBP() and checkD…
jimklimov 4f792f6
SCMSourceRetrieverTest.java: optimize tests using MBP to not build sa…
jimklimov 9d95881
SCMSourceRetrieverTest.java: adjust naming of tests using MBP [JENKIN…
jimklimov 0694dd1
SCMSourceRetrieverTest: add checkDefaultVersion_singleBranch() [JENKI…
jimklimov 824a204
SCMSourceRetrieverTest.java: report "Jobs generated by MBP" tests [JE…
jimklimov 553aba3
SCMSourceRetrieverTest.java: add PoC checkDefaultVersion_MBP_singleBr…
jimklimov bcba445
LibraryConfiguration: add optional traceBRANCH_NAME toggle, and lots …
jimklimov 6b5255c
LibraryConfigurationTest: enable traceBRANCH_NAME for test runs [JENK…
jimklimov a661a13
SCMSourceRetrieverTest: enable traceBRANCH_NAME for test runs [JENKIN…
jimklimov 9700565
SCMSourceRetrieverTest: update comments [JENKINS-69731]
jimklimov 30d21b9
SCMSourceRetrieverTest: rename some tests to follow a common pattern …
jimklimov 34718a4
SCMSourceRetrieverTest: separate checkDefaultVersion_inline_BRANCH_NA…
jimklimov 8e92f64
SCMSourceRetrieverTest: add checkDefaultVersion_MBPsingleBranch_BRANC…
jimklimov 5dc85dd
LibraryConfiguration: defaultedVersion(): unblock WorkflowRun handlin…
jimklimov a9aa8f1
SCMSourceRetrieverTest: checkDefaultVersion_inline_BRANCH_NAME(): tes…
jimklimov 418c572
SCMSourceRetrieverTest: add checkDefaultVersion_singleBranch_BRANCH_N…
jimklimov 54bb3e9
LibraryConfiguration: defaultedVersion(): add allowVersionEnvvar for …
jimklimov d47dad5
SCMSourceRetrieverTest: add PoC for checkDefaultVersion_inline_allowV…
jimklimov 811fb82
help-allowVersionEnvvar.html, help-allowBRANCH_NAME.html: stress that…
jimklimov 43d45b8
SCMSourceRetrieverTest: add checkDefaultVersion_singleBranch_BRANCH_N…
jimklimov 38f380f
LibraryConfiguration/config.jelly: fix markup [JENKINS-69731]
jimklimov 68902fd
LibraryConfiguration: doCheckDefaultVersion(): fix markup [JENKINS-69…
jimklimov dddd3ac
SCMSourceRetrieverTest: make checkDefaultVersion_inline_allowVersionE…
jimklimov 48ad0fe
LibraryConfiguration: defaultedVersion(): if there were no SCMs assoc…
jimklimov eeba94c
SCMSourceRetrieverTest: extend checkDefaultVersion_inline_allowVersio…
jimklimov ee36d66
LibraryConfiguration: defaultedVersion(): first check if WorkflowJob …
jimklimov 83cea07
SCMSourceRetrieverTest: checkDefaultVersion_MBPsingleBranch_*(): try …
jimklimov cf2c069
SCMSourceRetrieverTest: checkDefaultVersion_inline_allowVersionEnvvar…
jimklimov 6d74aea
LibraryConfiguration: refactor irky defaultedVersionSCM() helper out …
jimklimov 6097f6a
LibraryConfiguration and tests: rename traceBRANCH_NAME to more gener…
jimklimov 85e96e3
SCMSourceRetrieverTest: remove currently unused imports [JENKINS-69731]
jimklimov 4ce1164
SCMSourceRetrieverTest: complete the checkDefaultVersion_MBPsingleBra…
jimklimov ce8b820
SCMSourceRetrieverTest: checkDefaultVersion_inline_allowVersionEnvvar…
jimklimov 2d4bd48
SCMSourceRetrieverTest: checkDefaultVersion_inline_allowVersionEnvvar…
jimklimov 6767dc2
SCMSourceRetrieverTest: checkDefaultVersion_inline_allowVersionEnvvar…
jimklimov edd1ddb
SCMSourceRetrieverTest: checkDefaultVersion_inline_allowVersionEnvvar…
jimklimov 5190214
SCMSourceRetrieverTest: checkDefaultVersion_inline_allowVersionEnvvar…
jimklimov 06a3e09
SCMSourceRetrieverTest: checkDefaultVersion_inline_allowVersionEnvvar…
jimklimov f4c53c7
SCMSourceRetrieverTest: checkDefaultVersion_inline_allowVersionEnvvar…
jimklimov d529421
SCMSourceRetrieverTest: checkDefaultVersion_inline_allowVersionEnvvar…
jimklimov 71c51c8
SCMSourceRetrieverTest: checkDefaultVersion_inline_allowVersionEnvvar…
jimklimov 9e20d78
SCMSourceRetrieverTest: checkDefaultVersion_inline_allowVersionEnvvar…
jimklimov 3cb60c0
SCMSourceRetrieverTest: drop separate checkDefaultVersion_inline_allo…
jimklimov ba01128
SCMSourceRetrieverTest: add checkDefaultVersion_singleBranch_BRANCH_N…
jimklimov 863b170
LibraryConfiguration: defaultedVersionSCM(): avoid WorkflowJob.getSCM…
jimklimov dea552f
LibraryConfiguration: defaultedVersionSCM(): drop fallback WorkflowJo…
jimklimov 70b029c
LibraryConfiguration/config.jelly: update comment for allowVersionOve…
jimklimov 786092d
SCMSourceRetrieverTest: refactor baseline with sampleRepo1ContentMast…
jimklimov be0c051
SCMSourceRetrieverTest: refactor new tests with sampleRepo1Content*()…
jimklimov d67c2d1
SCMSourceRetrieverTest: clarify in surefire log that checkoutDirector…
jimklimov db74431
SCMSourceRetrieverTest.java: cleanup imports
jimklimov 812d12d
SCMSourceRetrieverTest.java: add checkDefaultVersion_singleBranch_BRA…
jimklimov 6ab574a
LibraryConfiguration.java: defaultedVersionSCM(): avoid get()ing from…
jimklimov 2d098a5
LibraryConfiguration.java: defaultedVersionSCM(): check for nuances o…
jimklimov 6379716
LibraryConfiguration.java: further refactor defaultedVersionSCM() to …
jimklimov ac9f40a
LibraryConfiguration.java: attempt to first extractDefaultedVersionSC…
jimklimov b95c46a
LibraryConfiguration.java: drop currently unnecessary imports
jimklimov a943713
LibraryConfiguration.java: extractDefaultedVersionSCMFS(): avoid redu…
jimklimov d039e50
LibraryConfiguration.java: extractDefaultedVersionSCMFS(): placeholde…
jimklimov 48548ae
Merge branch 'master' into JENKINS-69731-scm-BRANCH_NAME
jimklimov 5ab9713
Merge remote-tracking branch 'upstream/master' into JENKINS-69731-scm…
jimklimov 65a7d02
Merge branch 'master' into JENKINS-69731-scm-BRANCH_NAME
jimklimov c2cf4b5
Merge remote-tracking branch 'upstream/master' into JENKINS-69731-scm…
jimklimov 6668195
SCMSourceRetrieverTest: clarify messages for "SKIP by pre-test assump…
jimklimov c1082b9
SCMSourceRetrieverTest: provide directly a sampleRepoNotifyCommit() t…
jimklimov 02cf65b
Merge branch 'master' into JENKINS-69731-scm-BRANCH_NAME
jimklimov 7926f8c
Merge remote-tracking branch 'upstream/master' into JENKINS-69731-scm…
jimklimov 7329664
Merge remote-tracking branch 'upstream/master' into JENKINS-69731-scm…
jimklimov b8c4a88
Merge remote-tracking branch 'upstream/master' into JENKINS-69731-scm…
jimklimov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
763 changes: 761 additions & 2 deletions
763
src/main/java/org/jenkinsci/plugins/workflow/libs/LibraryConfiguration.java
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
17 changes: 17 additions & 0 deletions
17
...urces/org/jenkinsci/plugins/workflow/libs/LibraryConfiguration/help-allowBRANCH_NAME.html
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 | ||
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> |
10 changes: 10 additions & 0 deletions
10
...es/org/jenkinsci/plugins/workflow/libs/LibraryConfiguration/help-allowBRANCH_NAME_PR.html
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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> |
18 changes: 18 additions & 0 deletions
18
...ces/org/jenkinsci/plugins/workflow/libs/LibraryConfiguration/help-allowVersionEnvvar.html
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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> |
5 changes: 5 additions & 0 deletions
5
.../org/jenkinsci/plugins/workflow/libs/LibraryConfiguration/help-traceDefaultedVersion.html
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
So you could just use the
library
step except in cases where you need to refer to library types (src/
) using static typing, which forces use of the annotation. Definitely not the normal usage pattern.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.
src
and types (classes) defined there to wrap our SUTs (generic lockable resources passed around from stage to stage or even job to job => DBs => Mysql/Postgres/Oracle... and test VMs => vmware/qemu/...), some global static for shared/default config, and classes for custom tools + data-generator wrappers, etc. is indeed what we use. So thelibrary
step proved worse than useless after a week of trying, too fragile, depends on order we call steps and class methods/data, many of those are just not visible through proxy objects, etc.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
@Library('libname@somebranch')
differing in all the branches - kept spilling over as somebody branched their new feature from stable production code, and then the PR landed into qa or stage... it does not well allow for just fast-forward-merging the PRs.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.
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 "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.
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.
Also thinking of the
library
step to pass a groovy variableBRANCH_NAME
there - as I found with non-multibranch pipelines, at this time existence of this variable is not a given (see caveats and workarounds I posted just before your review). So one would have to copy-paste that bit around, or set up a library with one step to ensure it, just so thelibrary
step can know the "deemed correct" version to use in all cases? Sounds like a bigger hassle, if it can all be nailed here, and with all the benefits of the annotation :)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 is one way, though better I think is to have a consumer (repo with
Jenkinsfile
) temporarily consume the patched library repo with an explicit reference to the library branch, until that patch is well tested and merged into its default branch. Ideally with a way of mechanically blocking the PR to the consumer repo from being merged until the@Library
patch is reverted, so that you do not forget to follow the process. (I am using GitHub-oriented terms here but I think the style applies generally.) For the same reason, I would never advocate use of theresolveScm
step (jenkinsci/workflow-multibranch-plugin#46 (review) 🤷).One thing I have wanted for a while is a Dependabot module for Jenkins Pipeline libraries, so you could specify an (immutable!) tag in
@Library
but still keep up to date with improvements. This type of workflow is well established for GitHub Actions.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
LibraryRetriever
implementation, probably delegating toSCMSourceRetriever
likeSCMRetriever
does.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.
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.
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.
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).
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.
I am afraid that Jenkins is popular enough that, statistically, any requirement will be critical for somebody! The advantage of the
library
step is that you can script whatever logic makes sense for your setup without negotiating with a plugin developer over the details. I am wondering whether the step actually could be used effectively even for libraries which rely heavily on named types and static typing. Ultimately bothLibraryStep
andClasspathAdder
wind up callingGroovyClassLoader.addURL
, just at different times. If you want to refer to these types from code loaded fromJenkinsfile
at build initialization time then you must use@Library
. But AFAIK any Pipeline script sources loaded afterlibrary
has been run can also refer to those types. So it might work to useload
(orevaluate
+readTrusted
) to split the Groovy in the project repo into two parts, one small bootstrap section that determines a library to load, and the bulk of it in another section (in another source file?) that is parsed & compiled against that library. Could be checked in a functional test as a proof of concept. Awkward but at least a possibility for the advanced use cases.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.
So the pipeline source may not fully use classes defined by a
library
-step loaded lib, but can load another source which would be able to use those?Or is the idea to use
@Library
for a boot-strapper of sorts, which would decide which real library/version toload
/evaluate
/library
-step further - in context of the trusted (non-sandbox) library code?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.
@jglick @dwnusbaum : regarding security... maybe I'm missing something, but I've slept on the idea and concluded that the
library
step might allow use of arbitrary URLs for the library source (including the URL to repo with pipeline, possibly a malicious one), hence the step only provides untrusted sandboxing.In case of
@Library
the URL to repo is bolted by Jenkins admins in global config, at least for the trusted context (folders might let "users" add their own layers of libs, but untrusted - right?)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
allowBRANCH_NAME
for this library - or rather not if the population of writers into the repo is not under control)?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?)