cleanup request: replace all the `readYaml()` occurences by an agent command (`yq`, `sed`, whatever)

[dduportal]

The readYaml() feature is used on this repository as a pipelien step provided by https://github.com/jenkinsci/pipeline-utility-steps-plugin/blob/master/docs/STEPS.md.

But this step runs on the controller, wasting precious resources that could better be used on other tasks such as... handlin build orchestration and agents.

The request is to replace the readYaml() occurence by a call to an agent with command line tools such as yq (which is installed on all Linux agents and Windows VM agents).

[jglick]

From #30. Need to replace at least https://github.com/jenkins-infra/pipeline-library/blob/5288902cac5871e540ae9146e15e47cb47e6d757/vars/runATH.groovy#L41-L64.

Really this whole file should have been an external script. (It does use parallel, but apparently only within a single node, which is basically useless.) Furthermore, this library variable should not exist since it is only used in one place (https://github.com/jenkinsci/jenkins/blob/bd02516ef38e95d926b34a11e379d021545e5413/Jenkinsfile#L173) so it should be inlined into the jenkins repo.

[lemeurherve]

Regarding the execution of the ATH, should the build be done and tested with each jdk versions and not only jdk11 (https://github.com/jenkinsci/jenkins/blob/bd02516ef38e95d926b34a11e379d021545e5413/Jenkinsfile#L166)?

[jglick]

Sounds like overkill. More productive to include more tests in the smoke category. From my PoV the priority is to inline this library function, stripping out unused logic branches in the process.

[lemeurherve]

@raul-arabaolaza @jglick any idea why this was implemented here instead of the Jenkins repository in the first place?

I can see it useful for the infra team to be more easily modified, but then I'm not sure if it's the role of the infra team to deal with these tests implementations.

[jglick]

why this was implemented here

There was a whole plan which was never fully specified, much less implemented, and so we got some half-baked pieces introduced in a few places. https://github.com/jenkinsci/jep/blob/master/jep/309/README.adoc#bom-format etc.

I'm not sure if it's the role of the infra team to deal with these tests implementations

No, it is not.

[jglick]

Needs https://github.com/jenkinsci/jenkins/pull/7438 then https://github.com/jenkins-infra/pipeline-library/pull/525.

[lemeurherve]

I've removed the plugin from ci.jenkins.io, release.ci.jenkins.io and trusted.ci.jenkins.io

[basil]

I've removed the plugin from […] release.ci.jenkins.io

That will cause a failure of the next release, as the plugin is still used by the release process. Suggest restoring the plugin pending the identification and removal of all usages of the plugin.

[lemeurherve]

the plugin is still used by the release process

@basil, are you talking about other Jenkins branches like https://github.com/jenkinsci/jenkins/blob/stable-2.361/Jenkinsfile#L172?

I can't think of other place(s) and didn't find anything from my searches on @jenkinsci and @jenkins-infra, was it what you had in mind? If so, should all of them be inlined like https://github.com/jenkinsci/jenkins/pull/7438?

If there is more, can you point me to them please?

It also means we have to revert https://github.com/jenkins-infra/pipeline-library/pull/525 while it's still used.

[jglick]

I interpreted the comment as referring to the plugin, not this function. All I can find is https://github.com/jenkins-infra/release/blob/933208341833c9753740c1202d64018bf62980fc/Jenkinsfile.d/core/release#L120-L123 and https://github.com/jenkins-infra/release/blob/933208341833c9753740c1202d64018bf62980fc/Jenkinsfile.d/components/remoting#L88-L91 which seems mostly gratuitous (does anything ever read this variable?).

[lemeurherve]

Oh good catch, I missed these.

[jglick]

At worst, just replace

def properties = readProperties file: 'release/release.properties'
env.RELEASE_SCM_TAG = properties['scm.tag']

with

env.RELEASE_SCM_TAG = sh(returnStdout: true, script: 'fgrep scm.tag= release/release.properties | cut -c9-').trim()

[basil]

Yes, that is the usage that caused the failure of a recent weekly release during one of the days in the past few weeks when pipeline-utility-steps was not installed on release.ci.jenkins.io.

[dduportal]

Yes, that is the usage that caused the failure of a recent weekly release during one of the days in the past few weeks when pipeline-utility-steps was not installed on release.ci.jenkins.io.

I'm sorry, my brain totally forgot about this failure 2 weeks ago, that is correct. Thanks @basil for pointing this out!

[basil]

Found a couple usages of findFiles in the jenkins-infra organization as well. I did not do an exhaustive search of usages of the other steps offered by Pipeline Utility Steps. I suggest doing a systematic search of all consumers prior to removing this plugin.

[jglick]

BTW https://plugins.jenkins.io/plugin-usage-plugin/ will not help you here according to the warning in its docs. https://docs.cloudbees.com/docs/admin-resources/latest/plugin-management/plugin-usage would, if you could run it. Without that, I think you just need to go through https://www.jenkins.io/doc/pipeline/steps/pipeline-utility-steps/ and do a full-text search of script sources; or search build records on disk.

[basil]

readYaml still appears to be used in this repository as of HEAD at commit cd87a07:

$ git grep readYaml
test/groovy/BaseTest.groovy:    helper.registerAllowedMethod('readYaml', [Map.class], {
test/groovy/CustomWARPackagerStepTests.groovy:    helper.registerAllowedMethod('readYaml', [Map.class], {
test/groovy/CustomWARPackagerStepTests.groovy:    helper.registerAllowedMethod('readYaml', [Map.class], {
test/groovy/CustomWARPackagerStepTests.groovy:    helper.registerAllowedMethod('readYaml', [Map.class], {
test/groovy/CustomWARPackagerStepTests.groovy:    helper.registerAllowedMethod('readYaml', [Map.class], { m ->
vars/customWARPackager.groovy:  def metadata = readYaml(file: metadataFile)
vars/customWARPackager.groovy:    def bom = readYaml(file: bomFilePath)
vars/customWARPackager.groovy:    def config = readYaml(file: configFilePath)

[jglick]

Indeed. More cleanup coming…

[dduportal]

readYaml still appears to be used in this repository as of HEAD at commit cd87a07:

$ git grep readYaml
test/groovy/BaseTest.groovy:    helper.registerAllowedMethod('readYaml', [Map.class], {
test/groovy/CustomWARPackagerStepTests.groovy:    helper.registerAllowedMethod('readYaml', [Map.class], {
test/groovy/CustomWARPackagerStepTests.groovy:    helper.registerAllowedMethod('readYaml', [Map.class], {
test/groovy/CustomWARPackagerStepTests.groovy:    helper.registerAllowedMethod('readYaml', [Map.class], {
test/groovy/CustomWARPackagerStepTests.groovy:    helper.registerAllowedMethod('readYaml', [Map.class], { m ->
vars/customWARPackager.groovy:  def metadata = readYaml(file: metadataFile)
vars/customWARPackager.groovy:    def bom = readYaml(file: bomFilePath)
vars/customWARPackager.groovy:    def config = readYaml(file: configFilePath)

This one is "ok":

• test/** is only mocking the function
• vars/customWARPackager.groovy is not used as per @lemeurherve 's current researches and should soon be removed (incoming PR).

[dduportal]

Found a couple usages of findFiles in the jenkins-infra organization as well. I did not do an exhaustive search of usages of the other steps offered by Pipeline Utility Steps. I suggest doing a systematic search of all consumers prior to removing this plugin.

I have some doubts about the GitHub research tools: I swear I searched all the steps keyword and never found any except the ones in the pipeline-library. Now I see the same results as yours: it is really weird. I guess the lesson is: "next time, grepping and git-grepping on local git repositories as additional research could be valuable".

[jglick]

grepping and git-grepping on local git repositories

FWIW I use https://github.com/jenkins-infra/merge-all-repo/blob/master/clone.groovy for @jenkinsci. Rainy day project: create a gh CLI alias to check out / refresh an org checkout.

[lemeurherve]

you just need to go through jenkins.io/doc/pipeline/steps/pipeline-utility-steps and do a full-text search of script sources; or search build records on disk.

I've done that, previously on jenkins-infra/pipeline-library only, here are the results on @jenkinsci & @jenkins-infra:

in jenkins-infra:

1. ~https://cs.github.com/jenkins-infra/core-taglibs-report-generator/blob/55d672ad4520b3532a783e2731339dd6d759105b/Jenkinsfile?q=%2FfindFiles%7CprependToFile%7Ctouch%7Csha1%7Csha256%7CverifySha1%7CverifySha256%7Ctee%7Ctar%7Cuntar%7Czip%7Cunzip%7CreadProperties%7CreadManifest%7CreadYaml%7CwriteYaml%7CreadJSON%7CriteJSON%7CreadCSV%7CwriteCSV%7CreadMavenPom%7CwriteMavenPom%2F+language%3AGroovy#L60~ PR https://github.com/jenkins-infra/core-taglibs-report-generator/pull/27
2. ~https://cs.github.com/jenkins-infra/wechat/blob/2128a692a28f03c3dcda2410364eef845474d16e/Jenkinsfile.groovy?q=%2FfindFiles%7CprependToFile%7Ctouch%7Csha1%7Csha256%7CverifySha1%7CverifySha256%7Ctee%7Ctar%7Cuntar%7Czip%7Cunzip%7CreadProperties%7CreadManifest%7CreadYaml%7CwriteYaml%7CreadJSON%7CriteJSON%7CreadCSV%7CwriteCSV%7CreadMavenPom%7CwriteMavenPom%2F+language%3AGroovy#L53~ archived repo

In jenkinsci: 3) ~https://cs.github.com/jenkinsci/pipeline-examples/blob/fb9575a8182b51614f5f0df912b46b37d95fbb8d/declarative-examples/jenkinsfile-examples/mavenDocker.groovy?q=%2FfindFiles%7CprependToFile%7Ctouch%7Csha1%7Csha256%7CverifySha1%7CverifySha256%7Ctee%7Ctar%7Cuntar%7Czip%7Cunzip%7CreadProperties%7CreadManifest%7CreadYaml%7CwriteYaml%7CreadJSON%7CriteJSON%7CreadCSV%7CwriteCSV%7CreadMavenPom%7CwriteMavenPom%2F+language%3AGroovy~ example 4) ~https://cs.github.com/jenkinsci/jenkins-test-harness/blob/4865a72de2e61c09534473567fae2f92e47242bb/src/main/preset-data/package.groovy?q=%2FfindFiles%7CprependToFile%7Ctouch%7Csha1%7Csha256%7CverifySha1%7CverifySha256%7Ctee%7Ctar%7Cuntar%7Czip%7Cunzip%7CreadProperties%7CreadManifest%7CreadYaml%7CwriteYaml%7CreadJSON%7CriteJSON%7CreadCSV%7CwriteCSV%7CreadMavenPom%7CwriteMavenPom%2F+language%3AGroovy :warning:~ not Pipeline script

Other in the “don’t care” category in jenkinsci (?) 5) ~https://cs.github.com/jenkinsci/jenkinsfile-runner/blob/98f3b6911edacbeb7f97111bac9f83ea0785622d/demo/cwp/Jenkinsfile?q=%2FfindFiles%7CprependToFile%7Ctouch%7Csha1%7Csha256%7CverifySha1%7CverifySha256%7Ctee%7Ctar%7Cuntar%7Czip%7Cunzip%7CreadProperties%7CreadManifest%7CreadYaml%7CwriteYaml%7CreadJSON%7CriteJSON%7CreadCSV%7CwriteCSV%7CreadMavenPom%7CwriteMavenPom%2F+language%3AGroovy (we don’t care as~ in a “demo” folder ~I think?)~ 6) ~https://cs.github.com/jenkinsci/vectorcast-execution-plugin/blob/d266a469e3fcfab1ff34c6a39674ae537f06051b/src/main/resources/scripts/baseJenkinsfile.groovy?q=%2FfindFiles%7CprependToFile%7Ctouch%7Csha1%7Csha256%7CverifySha1%7CverifySha256%7Ctee%7Ctar%7Cuntar%7Czip%7Cunzip%7CreadProperties%7CreadManifest%7CreadYaml%7CwriteYaml%7CreadJSON%7CriteJSON%7CreadCSV%7CwriteCSV%7CreadMavenPom%7CwriteMavenPom%2F+language%3AGroovy~ not Pipeline script 7) ~https://cs.github.com/jenkinsci/btc-embeddedplatform-plugin/blob/5a5985ae176895b775bbb424f38d0191820c813a/src/main/resources/dsl/btc.groovy?q=%2FfindFiles%7CprependToFile%7Ctouch%7Csha1%7Csha256%7CverifySha1%7CverifySha256%7Ctee%7Ctar%7Cuntar%7Czip%7Cunzip%7CreadProperties%7CreadManifest%7CreadYaml%7CwriteYaml%7CreadJSON%7CriteJSON%7CreadCSV%7CwriteCSV%7CreadMavenPom%7CwriteMavenPom%2F+language%3AGroovy~ not Pipeline script n) diverse plugins without relation with infra

readYaml still appears to be used in this repository as of HEAD at commit https://github.com/jenkins-infra/pipeline-library/commit/cd87a074cbdcdb3072f7ee651afa5db7c8fdba40:

I've asked about this function usage on IRC, AFAIK it was for a defunct GSoC project.

[basil]

https://github.com/jenkins-infra/core-taglibs-report-generator/blob/55d672ad4520b3532a783e2731339dd6d759105b/Jenkinsfile#L60 seems to be the only actionable result from the above; the rest look like test code rather than production builds.

[jglick]

https://github.com/jenkinsci/jenkins-test-harness/blob/4865a72de2e61c09534473567fae2f92e47242bb/src/main/preset-data/package.groovy#L24-L41 is not Pipeline script, so irrelevant.

findFiles step is usually a code smell (should have inlined usage into some other shell script) but at worst could be replaced with the likes of

String[] files = sh(returnStdout: true, script: 'find dir -type f -print0').split('\u0000')

(Beware that this could return an array with one element, the empty string, if there are no matches.)

[lemeurherve]

I've replaced the last problematic uses of the plugin functions in these two pull requests, thanks to @jglick suggestions: https://github.com/jenkins-infra/core-taglibs-report-generator/pull/27 & https://github.com/jenkins-infra/release/pull/339

I've also searched for other usages in Jenkins Core and other "main" repositories (bom, ath, jth, casc), but didn't find anything.

WDYT if after announcing it on the jenkinsci-dev mailing list we remove https://github.com/jenkinsci/pipeline-utility-steps-plugin from ci.jenkins.io, seems good to you @jglick @basil @dduportal and others?

[jglick]

Can this be closed now or was there something left to do?

[lemeurherve]

Nothing left indeed, closing.