fix: stacker convert should check if FROMAS base is in current build - #2
fix: stacker convert should check if FROMAS base is in current build#2jmblatten wants to merge 1 commit into
Conversation
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>
| if fields[0] == "FROM" && len(fields) == 4 { | ||
| buildstages = append(buildstages, fields[3]) | ||
| } |
There was a problem hiding this comment.
This works for parsing FROM <image> AS <name>, but FROM --platform=<platform> <image> AS <name> is equally valid. https://docs.docker.com/reference/dockerfile/#from
Does Stacker handle multi-platform image references? If so, we should handle that use-case here.
There was a problem hiding this comment.
I would suggest len(fields) > 4 and fields[len(fields) - 1] which should cover both.
| var buildstages []string | ||
|
|
There was a problem hiding this comment.
I'm not sure if creating a new package-level variable is appropriate here. Perhaps create a local variable in parseFile() and pass it into convertCommand() instead as an argument?
| @@ -90,13 +93,22 @@ func (c *Converter) convertCommand(cmd *Command) error { | |||
| c.subs["IMAGE"] = "app" | |||
| } else if len(cmd.Value) == 3 && strings.EqualFold(cmd.Value[1], "as") { | |||
There was a problem hiding this comment.
This tells me that Stacker does not support --platform <platform> in FROM, so maybe you don't need to support it yet.
| } | ||
| // layer.BuildOnly = true // FIXME: should be enabled | ||
| } else { | ||
| return errors.Errorf("unsupported FROM directive") |
There was a problem hiding this comment.
And this tells me the --platform <platform> would be outright rejected. Please close the other two comments as resolved...
|
|
||
| // Check if the base stage is in a build stage | ||
| if slices.Contains(buildstages, cmd.Value[0]) { | ||
| layer.From.Type = "built" |
There was a problem hiding this comment.
Instead of a raw string, I would use the Stacker-defined type: https://github.com/project-stacker/stacker/blob/main/pkg/types/layer.go#L25
| layer.From.Type = "built" | ||
| layer.From.Tag = cmd.Value[0] | ||
| } | ||
| // layer.BuildOnly = true // FIXME: should be enabled |
There was a problem hiding this comment.
Have you looked into enabling this flag versus setting layer.From? Is there a difference between the two? Just wondering because this comment implies that they do keep track of these "build-only" stages using this flag.
I'm wondering if setting layer.From directly maybe bypasses some of that logic?
What type of PR is this?
bug
Which issue does this PR fix:
756
What does this PR do / Why do we need it:
See above.
If an issue # is not available please add repro steps and logs showing the issue:
Issue 756
myhello.go file contents,
Dockerfile contents:
Resulting, converted stacker.yaml,
Note that a stacker build will fail because there isn't a base layer A or B. They are instead stages in a build. Thus they perhaps should be represented as stages in the converted stacker file.
Testing done on this change:
Same Dockerfile and myhello.go from above.
stacker with fix, resulting converted stacker.yaml,
Note that A and B are tagged while mybuild and mybase pull in base images. stacker build should work.
Automation added to e2e:
None
Will this break upgrades or downgrades?
No
Does this PR introduce any user-facing change?:
No, because previously the converted file would not build.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.