-
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
Also read distro information from /etc/os-release when checking target compat #1352
Conversation
Signed-off-by: Natalie Arellano <narellano@vmware.com>
…t compat #1347 reads the file when providing target env vars to buildpacks during detect, but we also need to consider this info when deciding whether or not to run detect for the buildpack Signed-off-by: Natalie Arellano <narellano@vmware.com>
platform/target_data.go
Outdated
if base.OS == "" || base.Distro == nil { | ||
logger.Info("target distro name/version labels not found, reading /etc/os-release file") |
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 what cases could OS
be the empty string? If the author of the run image created it in such a way that the image didn't have properly specified metadata? The OCI spec says OS
is required:
https://github.com/opencontainers/image-spec/blob/main/config.md#properties
It's just at the moment, the log message (albeit whose wording exists prior to this PR) isn't consistent with the conditional. ie: The message says "target distro name/version labels", but the conditional suggests it should say "OS metadata, or target distro name/version labels" etc. (But if OS
should always be present, perhaps the OS
check can be moved to an earlier assertion that errors instead? then the log message wouldn't need updating)
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.
Good point. I added an error in 609e287
And remove checks for missing OS later in the build, as it should always be there Signed-off-by: Natalie Arellano <narellano@vmware.com>
Summary
This will fix an issue reported by our friends from Paketo. Without this fallback, when combined with the stricter validation in #1348, we could fail builds that we actually want to proceed with.
#1347 reads the file when providing target env vars to buildpacks during detect, but we also need to consider this info when deciding whether or not to run detect for the buildpack.
Release notes
The
detector
, when determining if a base image satisfies target constraints declared by a buildpack, also uses information from/etc/os-release
when distro information is not present in the run image labelsRelated
Resolves #___
Context
I moved some functions around in files, so ignore the first commit when parsing the diff (it just moves stuff doesn't change any logic)