Switch from `buildSrc/version.properties` to Gradle version catalog (`gradle/libs.versions.toml`) to enable dependabot to perform automated upgrades on common libs

[cwperks]

Description

Opening this PR in draft to explore what it will take to enable dependabot to perform automated upgrades on the dependency versions listed in buildSrc/version.properties.

This issue came up for discussion on my very first PR on the project: https://github.com/opensearch-project/OpenSearch/pull/3772.

Dependabot works on version catalogs: https://docs.github.com/en/code-security/dependabot/dependabot-version-updates/configuration-options-for-the-dependabot.yml-file#gradle

To test this I pushed to the main branch of my fork and used Dependabot CLI to run dependabot in a dry-run mode where it displays what PRs would be created without actually creating a PR.

To test, I created a sample dependabot configuration like this:

job:
  package-manager: gradle
  allowed-updates:
    - update-type: all
  source:
    provider: github
    repo: cwperks/opensearch
    directory: /

And ran it with ~/go/bin/dependabot update -f ./.github/dependabot_server.yml

See joda update in the output:

updater | +-------------------------------------------------------------------------------------------+
updater | |                            Changes to Dependabot Pull Requests                            |
updater | +---------+---------------------------------------------------------------------------------+
updater | | created | org.apache.ant:ant ( from 1.10.14 to 1.10.15 )                                  |
updater | | created | com.netflix.nebula:gradle-extra-configurations-plugin ( from 10.0.0 to 10.0.1 ) |
updater | | created | com.netflix.nebula:nebula-publishing-plugin ( from 21.0.0 to 21.1.0 )           |
updater | | created | com.netflix.nebula:gradle-info-plugin ( from 12.1.6 to 13.3.0 )                 |
updater | | created | org.apache.rat:apache-rat ( from 0.15 to 0.16.1 )                               |
updater | | created | net.java.dev.jna:jna ( from 5.14.0 to 5.15.0 )                                  |
updater | | created | com.avast.gradle:gradle-docker-compose-plugin ( from 0.17.6 to 0.17.9 )         |
updater | | created | org.apache.maven:maven-model ( from 3.9.6 to 3.9.9 )                            |
updater | | created | com.networknt:json-schema-validator ( from 1.2.0 to 1.5.2 )                     |
updater | | created | org.ajoberstar.grgit:grgit-core ( from 5.2.1 to 5.3.0 )                         |
updater | | created | org.wiremock:wiremock-standalone ( from 3.6.0 to 3.9.1 )                        |
updater | | created | org.spockframework:spock-core ( from 2.3-groovy-3.0 to 2.3-groovy-4.0 )         |
updater | | created | joda-time:joda-time ( from 2.12.7 to 2.13.0 )                                   |
updater | +---------+---------------------------------------------------------------------------------+

Related Issues

Resolves https://github.com/opensearch-project/OpenSearch/issues/3782

Check List

• [ ] Functionality includes testing.
• [ ] API changes companion pull request created, if applicable.
• [ ] Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions[bot]]

:x: Gradle check result for 724db1791a1dc1b55b6a02366c4ffdc337879c09: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[dbwiddis]

Opening this PR in draft to explore what it will take to enable dependabot to perform automated upgrades on the dependency versions listed in buildSrc/version.properties.

I recommend switching from Dependabot to using Mend Renovate.

I've outlined several reasons that it's a preferred choice in https://github.com/opensearch-project/.github/issues/97 and added a section to the Maintainer Responsibilities guide here.

One of the options of this configuration is to enable auto-merging for a subset of dependencies. I have done so on my own project here.

[cwperks]

One of the options of this configuration is to enable auto-merging for a subset of dependencies. I have done so on my own project here.

@peternied Has created something similar for dependabot and opensearch-trigger-bot PRs on the security repo. See the automatic-merges workflow here

[github-actions[bot]]

:x: Gradle check result for c831360a8c2a041c51e51500f3e09cc164b53364: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[cwperks]

There are some external references to the buildSrc/version.properties file like here

[github-actions[bot]]

:x: Gradle check result for 49ec14a26a8af3f9b1a184cc20113b53851388be: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions[bot]]

:x: Gradle check result for 41d3ab8c7d3dfaffa7d2e02221c1852874bc115d: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions[bot]]

:x: Gradle check result for 6ea86b6d0789910c19a9aeb8b7a5f23770e32038: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions[bot]]

:x: Gradle check result for 06840e2366c48ebbf24276318548eb3b3e9feff6: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions[bot]]

:white_check_mark: Gradle check result for 06840e2366c48ebbf24276318548eb3b3e9feff6: SUCCESS

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Please upload report for BASE (main@9f7d3b6). Learn more about missing BASE report. Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #16284   +/-   ##
=======================================
  Coverage        ?   72.18%           
  Complexity      ?    65145           
=======================================
  Files           ?     5313           
  Lines           ?   303439           
  Branches        ?    43909           
=======================================
  Hits            ?   219038           
  Misses          ?    66502           
  Partials        ?    17899           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[cwperks]

Curious to know from maintainers if there is appetite for a change like this, or if status quo with manual updates is sufficient. IMO automated updates help in a number of ways: 1) Less burden on maintainers, 2) better security by ensuring that dependency updates are not missed and 3) using the preferred method of defining common libs in gradle

[dbwiddis]

Curious to know from maintainers if there is appetite for a change like this, or if status quo with manual updates is sufficient. IMO automated updates help in a number of ways: 1) Less burden on maintainers, 2) better security by ensuring that dependency updates are not missed and 3) using the preferred method of defining common libs in gradle

I'd be in favor of automating patch version updates. Regarding (3) I'd really like to learn more about how plugins consume dpenendencies from a library like this. It seems we still have to declare our own versions, and worse, we don't get transitive dependencies automatically because they're so tightly controlled.

[github-actions[bot]]

:grey_exclamation: Gradle check result for b9e799d0812bf3d6d218267fe85cb6719d9f23e2: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[peternied]

@reta What do you think? You seem to be way more on top of version updates than me, commit history.

[reta]

@reta What do you think? You seem to be way more on top of version updates than me, commit history.

Thanks @peternied , it is certainly on my list, will try to get to it shortly (@cwperks sincere apologies)

[github-actions[bot]]

:white_check_mark: Gradle check result for eafb25e9afb8ad1900604f2d9469782329a24c4e: SUCCESS

[reta]

@cwperks I think it looks really great, there is only one issue I am concerned about. The versions catalog / TOML file is picked up and updated by dependabot, this is fine. The version.properties though (as of today) has to be regenerated manually (or with additional automation) every time TOML file is updated, right?

[cwperks]

@cwperks I think it looks really great, there is only one issue I am concerned about. The versions catalog / TOML file is picked up and updated by dependabot, this is fine. The version.properties though (as of today) has to be regenerated manually (or with additional automation) every time TOML file is updated, right?

@reta There is a gradle task in buildSrc/build.gradle called generateVersionProperties which generates the actual version.properties file. At first the version.properties file is written to buildSrc/build, but after its done generating this file it calls processResources like this:

processResources {
  from(generateVersionProperties)
}

This will copy the newly created file to buildSrc/build/resources/main/ which makes it available on the class path to be read in by VersionProperties

➜  OpenSearch git:(lib-toml) ✗ ls latr buildSrc/build
ls: latr: No such file or directory
buildSrc/build:
classes                  docs                     generated                pluginDescriptors        publications             resources                third-party-audit-config version.properties
distributions            forbidden-apis-config    markers                  pluginUnderTestMetadata  reports                  spotless                 tmp
➜  OpenSearch git:(lib-toml) ✗ ls latr buildSrc/build/resources/main/
ls: latr: No such file or directory
buildSrc/build/resources/main/:
META-INF                     eclipse.settings             fips_java_bcjsse_8.policy    fips_java_sunjsse.security   minimumCompilerVersion       opensearch.properties        version.properties
cacerts.bcfks                fips_java_bcjsse_11.policy   fips_java_bcjsse_8.security  forbidden                    minimumGradleVersion         plugin-descriptor.properties
deb                          fips_java_bcjsse_11.security fips_java_sunjsse.policy     license-headers              minimumRuntimeVersion        test

Downstream plugins can still keep their references to versions like this.

I also added an example of adding [libraries] section of the gradle catalog in this PR and showed how it can be used to reference entries in the gradle catalog directly (f.e. api(libs.jodatime)). Unfortunately, I don't think we can use the same references in downstream plugins and they would need to keep referencing versions.

[reta]

@reta There is a gradle task in buildSrc/build.gradle called generateVersionProperties which generates the actual version.properties file.

Thanks @cwperks , I totally get this part, I am trying to reconstruct the flow, may be a detailed example would be helpful:

• the dependabot opens the pull request (it only updates TOML / version catalog, version.properties stays unchanged)
• once dependabot's pull request is merged, the publication of the snapshots happens where the version.properties file is regenerated (and published)

Is my understanding correct? Thank you.

[cwperks]

Yes, you are right. It is included in the build-tools jar.

For example, the build-tools snapshots are located here: https://aws.oss.sonatype.org/content/repositories/snapshots/org/opensearch/gradle/build-tools

Edit: Just found developer docs on this: https://github.com/opensearch-project/opensearch-plugins/blob/8febf76d863a9608e508603e9b9335b2bc91e437/BUILDING.md?plain=1#L196-L210

[reta]

Yes, you are right. It is included in the build-tools jar.

Awesome, thank you. I am wondering if we could completely phase out versions.properties and use versions catalog / TOML instead (since it is fenced by VersionProperties anyway)? (this is just a question, I think we could play safe and always publish the generated version).

[cwperks]

Yes, you are right. It is included in the build-tools jar.

Awesome, thank you. I am wondering if we could completely phase out versions.properties and use versions catalog / TOML instead (since it is fenced by VersionProperties anyway)? (this is just a question, I think we could play safe and always publish the generated version).

I would like to see if its possible to consume the new gradle version catalog downstream similar to how the version properties is consumed by plugins.

I like the simplicity:

[versions]
joda              = "2.12.7"

[libraries]
jodatime = { group = "joda-time", name = "joda-time", version.ref = "joda" }

and in dependencies section of build.gradle:

dependencies {
    api(libs.jodatime)
}

^ With this PR we can do this in any internal module, but not in downstream plugins. For downstream plugins, this PR maintains the publication of the version.properties that is generated from ./gradlew generateVersionProperties

The gradle version catalog will be really useful in the future because we can also define bundles of dependencies. See https://docs.gradle.org/current/userguide/platforms.html#sub::toml-dependencies-format for what's possible with the version catalog.

[cwperks]

To completely phase it out, we need to remove references to the version.properties file outside this repo. For instance, I found a reference here

[reta]

I would like to see if its possible to consume the new gradle version catalog downstream similar to how the version properties is consumed by plugins.

Yes, I think that would be a very logical next step. I am debating if we should be using TOML file or codified settings.gradle (https://github.com/opensearch-project/spring-data-opensearch/blob/main/settings.gradle.kts) but it seems like dependabot does not support versions update for version declarations (defeats the purpose).

To completely phase it out, we need to remove references to the version.properties file outside this repo. For instance, I found a reference here

This one references the buildSrc/version.properties, not a generated one, right?

[cwperks]

This one references the buildSrc/version.properties, not a generated one, right?

Yep, that's why this PR keeps the file checked in but removes everything other than opensearch

[dbwiddis]

The gradle version catalog will be really useful in the future

Yes.

[reta]

@cwperks OK, I think I finally glued all pieces together :D (sorry it took so long). So here are 3 parts (or stages) that we have to go through to have dependency management revamped in favour of using versions catalog + be dependabot friendly:

1. Add libs.versions.toml with only [versions] section (version catalog), the dependabot would do nothing with that at this stage
2. Generate version.properties out of libs.versions.toml (so nothing changes there by and large for consumers)
3. Add all dependencies to libs.versions.toml's [libraries] section: that would centralize the dependency management and actually would let dependabot to pick version updates

Does it make sense? If yes, I would like to ask you remove [libraries] section from the TOML file (only keep [versions]) so we could minimize any possible (unknown) risks of breaking things. Thanks!

[cwperks]

@reta I removed the [libraries] section, but kept the logic in place that parses the [versions] section from the TOML file to generate the version.properties file. That way if we add additional sections in the toml file in the future, the logic to handle it is already in place.

[reta]

@reta I removed the [libraries] section, but keeped the logic in place that parses the [versions] section from the TOML file to generate the version.properties file. That way if we add additional sections in the toml file in the future, the logic to handle it is already in place.

@cwperks about that, do you mind if I push a small change to this particular part? I prefer us to not do manual parsing. Thank you.