Skip to content

Add convert fromas testcase - #4

Closed
jmblatten wants to merge 3 commits into
mainfrom
add-convert-fromas-testcase
Closed

jmblatten wants to merge 3 commits into
mainfrom
add-convert-fromas-testcase

Conversation

@jmblatten

Copy link
Copy Markdown
Collaborator

What type of PR is this?

Which issue does this PR fix:

What does this PR do / Why do we need it:

If an issue # is not available please add repro steps and logs showing the issue:

Testing done on this change:

Automation added to e2e:

Will this break upgrades or downgrades?

Does this PR introduce any user-facing change?:


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jmblatten
jmblatten force-pushed the add-convert-fromas-testcase branch 8 times, most recently from 8c33413 to e441e0e Compare September 17, 2026 22:17
    Currently, stacker converts the base part of a "FROM base AS name"
    found in a dockerfile to stacker's from directive with type:docker
    and url:docker.
    However, the base could be a build stage in the current session.
    In that case, stacker's from directive should be type:built and
    tag:<something> instead.

Signed-off-by: Joy Latten <joylatten@geico.com>
@jmblatten
jmblatten force-pushed the add-convert-fromas-testcase branch from e441e0e to 0107837 Compare October 2, 2026 17:30
Signed-off-by: Joy Latten <joylatten@geico.com>
@jmblatten
jmblatten force-pushed the add-convert-fromas-testcase branch from 0107837 to a15b729 Compare October 2, 2026 17:52
Signed-off-by: Joy Latten <joylatten@geico.com>

@jsatterfield-geico jsatterfield-geico left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jmblatten

Copy link
Copy Markdown
Collaborator Author

This PR is for internal review only. It was opened back in September believing mirrors were not ready for general use. It was also opened in response to feedback for upstreamed PR. (See
project-stacker#785)

My intentions are that once reviewed, I will only push the testcase commit(s) to the already upstreamed PR. This PR will then be closed without merging, since only for internal review.

Note: Currently, the mirror fails to sync with upstream, so am asking for internal review here.

@PinkMonstera PinkMonstera left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No obvious security concerns. SSCS approved

@jmblatten
jmblatten marked this pull request as ready for review October 20, 2026 18:05

@fusion94 fusion94 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm approving but there is a correctness issue that will probably need to be addressed in the future:

Plain FROM <stage> with no AS isn't handled. The new check only runs in the len(cmd.Value) == 3 branch (convert.go:91). This pattern is very common:
FROM golang AS build
...
FROM build
The final stage still gets type: docker, url: docker://build, which fails at build time. Most real multi-stage Dockerfiles end with a final stage that has no AS, so the fix misses the most common case.

@jmblatten

Copy link
Copy Markdown
Collaborator Author

Hi @fusion94,
Thanks for the review!
From what I gleaned from docs, https://docs.docker.com/reference/dockerfile/#from,
a lone FROM would be proceeded with an image and not a stage-name.
i.e. FROM . And stacker convert handled that ok. But stacker convert mis-handled a FROM that included a stage-name.
The stacker convert is literal and assumes the Dockerfile is correct.

@jmblatten jmblatten closed this Oct 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants