Upgrade ARG default values used in Docker FROM instructions - #1213
Merged
Conversation
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`.
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.
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.
timtebeek
marked this pull request as draft
August 20, 2026 15:20
timtebeek
commented
Aug 20, 2026
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.
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.
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.
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.
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.
Member
Author
|
Currently exploring a registry method in the DockerImageReference trait, to make it easier to match & change. |
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.
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
marked this pull request as ready for review
August 21, 2026 23:49
This was referenced Aug 22, 2026
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.
FROMimages built from variables #1212.What's changed
FROMimages built from variables #1212 madeUpgradeDockerImageVersionskip anyFROMwhose image name or tag contains a$, since the value can not be determined statically. That is the right call for a bareARG IMAGE_TAG, but a great many Dockerfiles pin the Java version in a globalARGwith a default value:Here the value is statically known, and the version can be bumped by rewriting the
ARGdefault rather than theFROM.The visitor now collects the literal defaults of the global
ARGs (those before the firstFROM, which are the only ones aFROMcan reference) and resolves variable image references against them. When the resolved reference is an upgradable Java image, it writes the upgrade back to whichever of the two carries the value:ARG java_version=17FROM eclipse-temurin:${java_version}ARG java_version=25FROM eclipse-temurin:${java_version}ARG JAVA_VERSION=11FROM eclipse-temurin:${JAVA_VERSION}-jreARG JAVA_VERSION=25FROM eclipse-temurin:${JAVA_VERSION}-jreARG IMAGE_TAG=11-jre-alpineFROM eclipse-temurin:${IMAGE_TAG}ARG IMAGE_TAG=25-jre-alpineFROM eclipse-temurin:${IMAGE_TAG}ARG BASE_IMAGE=openjdk:11-jreFROM ${BASE_IMAGE}ARG BASE_IMAGE=eclipse-temurin:25-jreFROM ${BASE_IMAGE}ARG BASE_IMAGE=openjdkFROM ${BASE_IMAGE}:11-jreARG BASE_IMAGE=eclipse-temurinFROM ${BASE_IMAGE}:25-jreNote the last two: an
openjdkbase has no tag beyond 17, so the deprecated image name has to move toeclipse-temurinalong with the tag, whether that name sits in theFROMor in theARG.Everything that can not be resolved is still left untouched: an
ARGwithout a default, anARGwhose default is itself built from another variable, a variable that only contributes part of the image name (FROM ${REGISTRY}/eclipse-temurin:11-jre), a variable that does not supply the leading version (FROM eclipse-temurin:11${SUFFIX}), and anyARGdeclared after the firstFROM.ARGs that are not used in aFROMat all are never rewritten. As before, a digest pin is dropped when the tag is upgraded.The fully literal
FROMpath is unchanged.Tests
Added parameterized cases for each row of the table above, plus multi-stage
ARGreuse, digest pin removal through anARG, and a set of negative cases covering the unresolvable and unrelated shapes. The existing tests are unchanged and still pass.