Do not rewrite Docker FROM images built from variables - #1212
Merged
Conversation
`UpgradeDockerImageVersion` composed a `ChangeFrom` per (image, oldVersion)
pair. `DockerFrom`'s matcher treats an environment variable as a `*` wildcard
and matches bidirectionally, so `FROM ${IMAGE_NAME}:${IMAGE_TAG}` matched every
one of those globs and was rewritten to a hardcoded `eclipse-temurin:25`.
Replace the ~170 sub-recipes with a single visitor that reads the image name and
tag, skips any reference containing a variable, and bumps only tags whose
leading version is between 8 and the target version.
The digest, not the tag, decides which image is pulled, so carrying the old digest over would keep resolving to the pre-upgrade image.
timtebeek
marked this pull request as ready for review
August 20, 2026 13:43
timtebeek
added a commit
that referenced
this pull request
Aug 21, 2026
* Upgrade `ARG` default values used in Docker `FROM` instructions Follow up to #1212, which left any `FROM` built from a variable untouched. When the variable is a global `ARG` with a literal default, that default can be upgraded instead, so `ARG java_version=17` used as `FROM eclipse-temurin:${java_version}` becomes `ARG java_version=25`. * Do not bump `ARG` defaults shared with images we leave alone Three follow ups from review: - A `FROM` whose image we do not upgrade now vetoes the arguments feeding it, so `ARG VERSION=11` used by both `eclipse-temurin:${VERSION}` and `node:${VERSION}` is left alone rather than turning the latter into `node:25`. - Drop the digest pin when an argument holding a whole `name:tag` reference is upgraded, as the stale digest would keep resolving to the old image. - Upgrade quoted default values, keeping their quotes. The parser hands an `ARG` value to us as a single literal with the quotes still in its text and no quote style, so `ARG JAVA_VERSION="11"` never matched a version before. * Trim comments and javadoc that restate the code * Plan `ARG` upgrades before rewriting any `FROM` Rewriting a `FROM` as it was visited, and only withholding the matching `ARG` bump after the traversal, left half applied edits behind: a dropped digest pin or a rename to `eclipse-temurin` next to an argument still holding the old version. The whole file is now planned up front, and replayed until the set of withheld arguments stops growing, as withholding one argument can rule out the images that depend on it. Only the surviving plan is applied. Every give up path now withholds the arguments that `FROM` reads. * Read `ARG` default values through the new `Argument` accessors openrewrite/rewrite#8576 routes `ARG` values through the same path as every other argument, so quotes are modelled in `Literal.quoteStyle` and `$VAR` references become `EnvironmentVariable`. The local `QuotedText` that took the quotes off the literal text by hand is no longer needed, and reading a value is now `Argument.getText()`, which gives up on a reference the same way. * Leave a `FROM` with a dangling tag separator alone openrewrite/rewrite#8590 splits an image reference in the grammar, so `FROM eclipse-temurin:` now parses rather than failing. Cover the shape that newly reaches the recipe. * Do not bump `ARG` defaults shared with images we leave alone An argument holding only an image name carries no version, so renaming a deprecated image holds for every `FROM` that reads it, whatever tag each one carries. Withhold a version alone: ARG BASE=openjdk ARG BASE=eclipse-temurin FROM ${BASE}:11-jre -> FROM ${BASE}:25-jre FROM ${BASE}:latest FROM ${BASE}:latest A `FROM` whose image we can not resolve, or is not one we upgrade, still withholds both, as we know too little to rename either. Hoist the recipe into `defaults`, leaving the one test that varies the target version to override it. * Bump an `ARG` default shared with an image we leave alone An image we do not recognise may well be an internal one built on the same Java version, and if it shared a version argument before it should move along with it: ARG VERSION=11 ARG VERSION=25 FROM eclipse-temurin:${VERSION} -> FROM eclipse-temurin:${VERSION} FROM acme/base:${VERSION} FROM acme/base:${VERSION} Each `FROM` now stands on its own, so the repeated planning that let one withheld argument withhold the next is gone, and a visitor reads the file in one pass. Group the tests that assert no change into a nested class. * Upgrade a Java image pulled through a registry Mirroring stock images through an internal registry is ordinary, so read the repository past a registry rather than treating the whole reference as a name we do not know: ARG REGISTRY ARG REGISTRY FROM ${REGISTRY}/eclipse-temurin:11-jre -> FROM ${REGISTRY}/eclipse-temurin:25-jre FROM docker.io/openjdk:11-jre FROM docker.io/eclipse-temurin:25-jre Which leading segment is a registry follows Docker's own rule: one holding a `.` or a `:`, or `localhost`. Anything else belongs to the repository, so `azul/zulu-openjdk` still reads as a name and `mycompany/eclipse-temurin` is left alone. Also rewrite only the `ARG` declaration a default was read from, as a name may be declared more than once and the others carry no value to upgrade. * Read a registry through rewrite-docker's `ImageName` The rule for which leading segment of an image name is a registry now lives upstream, in `org.openrewrite.docker.trait.ImageName`, so read the registry and repository from it rather than keeping a second copy of the rule here. That also covers a segment upstream reads as a registry and the copy here did not: a repository name may not be uppercase, so `MyRegistry/openjdk` names a registry, not a repository. Reading the name through `getTextWithVariables()` lets one parse serve a spelt-out registry and a `${REGISTRY}` alike, which retires the second helper that recognised the latter by the shape of its contents. * Upgrade a `FROM` as it is visited Planning every `FROM` before rewriting any was there to withhold an argument another `FROM` ruled out. Nothing is withheld now, so each one can be upgraded where it is met, which retires the second traversal, the plan holder and the map of replacements keyed by id. Read the declaration an argument's default came from by its value rather than by its id, so the two maps become one.
timtebeek
added a commit
that referenced
this pull request
Aug 22, 2026
…ns (#1213)" (#1215) This reverts commit d33eca3. The recipe reached for rewrite-docker API that no released version of rewrite carries yet. `Docker.Argument.getText()`, `getTextWithVariables()` and `hasEnvironmentVariables()` arrived in openrewrite/rewrite#8576, and `org.openrewrite.docker.trait.ImageName` in openrewrite/rewrite#8599, both landed 2026-08-21, one day after v8.90.3. The Moderne CLI loads the LST classes in its own classloader, so a recipe runs against the rewrite-docker the CLI bundles rather than the one this artifact resolves. CLI 4.6.3 bundles rewrite-docker 8.90.3, where `Docker.Argument` exposes only `getContents()` and `ImageName` does not exist. `visitFile` reads a global `ARG` through `getText()` and `visitFrom` opens with `hasEnvironmentVariables()`, so the first `FROM` of every Dockerfile would raise `NoSuchMethodError`, surfacing as error markup on every Dockerfile of every `UpgradeToJava*` run. Nothing is lost by waiting: v3.42.1 predates this commit, so the breakage has not shipped. The state restored here is the #1212 fix, which reads an image reference through `DockerFrom` alone and so links against 8.90.3. Reapplied in a follow-up PR, to merge once a rewrite release carries #8576, #8590 and #8599 and the CLI picks it up.
timtebeek
added a commit
that referenced
this pull request
Aug 22, 2026
… for rewrite release) (#1216) * Revert "Upgrade `ARG` default values used in Docker `FROM` instructions (#1213)" This reverts commit d33eca3. The recipe reached for rewrite-docker API that no released version of rewrite carries yet. `Docker.Argument.getText()`, `getTextWithVariables()` and `hasEnvironmentVariables()` arrived in openrewrite/rewrite#8576, and `org.openrewrite.docker.trait.ImageName` in openrewrite/rewrite#8599, both landed 2026-08-21, one day after v8.90.3. The Moderne CLI loads the LST classes in its own classloader, so a recipe runs against the rewrite-docker the CLI bundles rather than the one this artifact resolves. CLI 4.6.3 bundles rewrite-docker 8.90.3, where `Docker.Argument` exposes only `getContents()` and `ImageName` does not exist. `visitFile` reads a global `ARG` through `getText()` and `visitFrom` opens with `hasEnvironmentVariables()`, so the first `FROM` of every Dockerfile would raise `NoSuchMethodError`, surfacing as error markup on every Dockerfile of every `UpgradeToJava*` run. Nothing is lost by waiting: v3.42.1 predates this commit, so the breakage has not shipped. The state restored here is the #1212 fix, which reads an image reference through `DockerFrom` alone and so links against 8.90.3. Reapplied in a follow-up PR, to merge once a rewrite release carries #8576, #8590 and #8599 and the CLI picks it up. * Upgrade `ARG` default values used in Docker `FROM` instructions Reapplies #1213, reverted in #1215 because the rewrite-docker API it reads `ARG` defaults through had not been released yet. Hold until a rewrite release carries openrewrite/rewrite#8576 (the `Docker.Argument` accessors), #8590 (the image reference grammar) and #8599 (`ImageName`), and confirm the Moderne CLI bundles that release, as the CLI loads the LST classes in its own classloader and so decides which rewrite-docker a recipe actually links against. * Revert "Merge branch 'main' into tim/docker-arg-defaults-redo" This reverts commit 901210a, keeping the branch side. Merging main in pulled #1215 across, and #1215 is the revert of the very commit this branch exists to reapply. The merge base still carried the `ARG` work and main had removed it, so the merge resolved to main's removal and emptied the branch: `git diff main...HEAD` came back with nothing, leaving the pull request proposing no change at all. Reverting the merge rather than dropping it also settles the branch. The merge stays in history, so main's revert counts as already merged here and undone on purpose. A later `main` merge brings its new commits without resurrecting the removal, which resetting the branch would leave it open to on the next `Update branch`. * Read an argument's text through `ArgumentContents` openrewrite/rewrite#8608 moves `getText()`, `getTextWithVariables()`, `getQuoteStyle()` and `hasEnvironmentVariables()` off `Docker.Argument` and into `org.openrewrite.docker.internal.ArgumentContents`, then drops them from the LST type. That matters because of how the Moderne CLI splits one rewrite-docker jar across two classloaders: `org.openrewrite.docker.tree` resolves to the rewrite-docker the CLI bundles, while recipes, traits and `internal` load child-first from the recipe artifact. Reading an argument through the LST type therefore linked against the CLI's copy, which is where #1215 came from. The helpers now sit on the recipe's side of that split and read only members that predate the CLIs in the field, so they travel with this artifact. `ImageName` already sits on that side, so it needed no change. Verified against the pull request rather than assumed: rewrite-docker built at 6f5fd253 and published locally, and every one of the fifteen `org.openrewrite.docker.tree` members this recipe links against confirmed present in the 8.90.3 jar CLI 4.6.3 bundles, matching on descriptor.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fixes #1211
What's changed
UpgradeDockerImageVersionbuilt aChangeFromsub-recipe for every (image, oldVersion) pair — 170 of them when targeting Java 25.DockerFrom's matcher substitutes a*for each environment variable and then matches bidirectionally, so forthe image name matched as
matchesGlob("openjdk", "*")and the tag asmatchesGlob("8*", "*"). The firstChangeFromin the list therefore fired, the$1capture resolved to nothing, and the reference was replaced with a hardcodedeclipse-temurin:25.This replaces the sub-recipe list with a single visitor that:
FROMwhose image name or tag contains a$, since the value can not be determined statically;-jre-alpine,-jdk-jammy, ...);openjdk/adoptopenjdktoeclipse-temurin;eclipse-temurin:25-jre@sha256:<openjdk-11-digest>would keep resolving to the pre-upgrade image.As a side effect the recipe list of the aggregate migrations shrinks considerably:
UpgradeToJava25drops from 1398 to 488 recipes.Tests
Added cases covering
${VAR}and$VARin the image name, the tag, and both; a Dockerfile that mixes a variableFROMwith a literal one; unrelated images; and digest pin removal. The existing parameterized cases are unchanged and still pass.