-
Notifications
You must be signed in to change notification settings - Fork 104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
More fixes for target compat checking during detect #1354
Conversation
- If a buildpack fails to specify os/arch (but specifies distro) still check targets - If the run image fails to specify os/arch (this should not happen actually as we will fail during analyze) still check targets - Fix typo in buildpack descriptor struct so that we actually get stack information - If we get distro information from /etc/os-release, persist this information to later invocations to that the log message printed when errors are encountered will be accurate - Don't override inner `i` in loop (this should not actually affect the outer loop but is confusing) Signed-off-by: Natalie Arellano <narellano@vmware.com>
Missing targets is sufficient for wildcard match Signed-off-by: Natalie Arellano <narellano@vmware.com>
@@ -70,8 +70,6 @@ func ReadBpDescriptor(path string) (*BpDescriptor, error) { | |||
for _, stack := range descriptor.Stacks { | |||
if stack.ID == "io.buildpacks.stacks.bionic" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add jammy
given it's the primary stack that paketo supports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed - we will opt for a len == 1 && stack == bionic check, as bionic is encoded in the spec.
targetMatch := false | ||
if isWildcard(d.AnalyzeMD.RunImageTarget()) || hasWildcard(descriptor.TargetsList()) { | ||
var targetMatch bool | ||
if len(descriptor.TargetsList()) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this for backwards-compatibility with buildpacks that don't have a targets
field? Is this spec-compliant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In reality, this is for tests. In practice, the lifecycle will infer at least an OS when targets are missing. Here is what the spec has to say about matching:
If the targets list is empty, tools reading buildpack.toml will assume:
os = "linux" and arch = if ./bin/build is present
os = "windows" and arch = if ./bin/build.bat or ./bin/build.exe are present
Metadata specified in [[targets]] is validated against the runtime and build-time base images.
A buildpack target satisfies a base image target when os, arch, and variant match and at least one distribution in distros (if provided) matches
… succeed Fixes #1355 Signed-off-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Natalie Arellano <narellano@vmware.com>
@@ -1,4 +1,4 @@ | |||
FROM ubuntu:bionic | |||
FROM ubuntu:jammy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change causes
lifecycle/acceptance/detector_test.go
Line 163 in 1a1de08
it("writes group.toml and plan.toml at the default locations", func() { |
[[stacks]]
id = "io.buildpacks.stacks.bionic"
[[stacks]]
id = "io.buildpacks.stacks.jammy"
is added to acceptance/testdata/detector/container/cnb/buildpacks/simple_buildpack/simple_buildpack_version/buildpack.toml without corresponding changes in how we read the descriptor
@@ -17,7 +18,7 @@ type BpDescriptor struct { | |||
Order Order `toml:"order"` | |||
WithRootDir string `toml:"-"` | |||
Targets []TargetMetadata `toml:"targets"` | |||
Stacks []StackMetadata `tome:"stacks"` // just for backwards compat so we can check if it's the bionic stack, which we translate to a target | |||
Stacks []StackMetadata `toml:"stacks"` // just for backwards compat so we can check if it's the bionic stack, which we translate to a target |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth noting that this typo while unsightly didn't cause errors because the struct field name matches the expected TOML key name
@@ -144,26 +144,14 @@ func (d *Detector) detectOrder(order buildpack.Order, done, next []buildpack.Gro | |||
return nil, nil, ErrFailedDetection | |||
} | |||
|
|||
// isWildcard returns true IFF the Arch and OS are unspecified, meaning that the target arch/os are "any" | |||
func isWildcard(t files.TargetMetadata) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice, isWildcard
would never return true because we check that we at least have an OS during analyze (also OS/arch is required in the OCI spec)
return t.Arch == "" && t.OS == "" | ||
} | ||
|
||
func hasWildcard(ts []buildpack.TargetMetadata) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible to have a target without OS/arch but with a distribution so this function never made much sense
func (d *Detector) detectGroup(group buildpack.Group, done []buildpack.GroupElement, wg *sync.WaitGroup) ([]buildpack.GroupElement, []files.BuildPlanEntry, error) { | ||
// used below to mark each item as "done" by appending it to the done list | ||
markDone := func(groupEl buildpack.GroupElement, descriptor buildpack.Descriptor) { | ||
done = append(done, groupEl.WithAPI(descriptor.API()).WithHomepage(descriptor.Homepage())) | ||
} | ||
|
||
runImageTargetInfo := d.AnalyzeMD.RunImageTarget() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so we can keep track of the info we read from /etc/os-release and print a more meaningful log message
Summary
so that we actually get stack information(we still see it because of the struct field name)i
in loop (this should not actually affect the outer loop but is confusing)Release notes
detector
, when checking target compatibility, will still perform the check if buildpacks fail to declare os/arch information in targets