Skip to content
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

Buildpack API deprecation warning not shown for transitive dependency buildpacks #1248

Open
edmorley opened this issue Nov 15, 2023 · 3 comments
Labels
help wanted Need some extra hands to the this done. status/ready type/bug Something isn't working

Comments

@edmorley
Copy link
Contributor

Summary

If a buildpack is using a deprecated Buildpack API version, lifecycle outputs a warning like so during detect:

===> DETECTING
Warning: Buildpack 'heroku/[email protected]' requests deprecated API '0.6'

However, if the buildpack in question isn't in the top level buildpack group, and instead is a transitive dependency of a composite buildpack, then the warning isn't shown.

It seems that the check here isn't traversing the full buildpack order group graph:

func verifyBuildpackApis(group buildpack.Group) error {
for _, bp := range group.Group {
if err := cmd.VerifyBuildpackAPI(buildpack.KindBuildpack, bp.String(), bp.API, cmd.DefaultLogger); err != nil { // FIXME: when exporter is extensions-aware, this function call should be modified to provide the right module kind
return err
}
}
return nil
}

lifecycle/cmd/apis.go

Lines 35 to 66 in dbd27bd

// VerifyBuildpackAPI given a Buildpack API version and relevant information for logging
// will error if the requested version is unsupported,
// and will log a warning, error, or do nothing if the requested version is deprecated,
// depending on if the configured deprecation mode is "warn", "error", or "silent", respectively.
func VerifyBuildpackAPI(kind, name, requestedVersion string, logger log.Logger) error {
requested, err := api.NewVersion(requestedVersion)
if err != nil {
return FailErrCode(
nil,
CodeForIncompatibleBuildpackAPI,
fmt.Sprintf("parse buildpack API '%s' for %s '%s'", requested, strings.ToLower(kind), name),
)
}
if api.Buildpack.IsSupported(requested) {
if api.Buildpack.IsDeprecated(requested) {
switch DeprecationMode {
case ModeQuiet:
break
case ModeError:
logger.Errorf("%s '%s' requests deprecated API '%s'", kind, name, requestedVersion)
logger.Errorf("Deprecated APIs are disabled by %s=%s", EnvDeprecationMode, ModeError)
return buildpackAPIError(kind, name, requestedVersion)
case ModeWarn:
logger.Warnf("%s '%s' requests deprecated API '%s'", kind, name, requestedVersion)
default:
logger.Warnf("%s '%s' requests deprecated API '%s'", kind, name, requestedVersion)
}
}
return nil
}
return buildpackAPIError(kind, name, requestedVersion)
}


Reproduction

Steps
  1. mkdir testcase && cd $_
  2. pack build test --builder heroku/builder-classic:22 --buildpack heroku/[email protected]
  3. pack build test --builder heroku/builder-classic:22 --buildpack heroku/[email protected]
Current behavior

At step 2 (where the buildpack is referenced directly), a deprecation warning is correctly shown:

===> DETECTING
Warning: Buildpack 'heroku/[email protected]' requests deprecated API '0.6'
Timer: Detector started at 2023-11-15T08:45:46Z
======== Results ========
fail: heroku/[email protected]

At step 3 (where the buildpack with the deprecated Buildpack API version is instead a transitive dependency of the composite buildpack heroku/nodejs-function), the deprecation warning is missing:

===> DETECTING
Timer: Detector started at 2023-11-15T08:45:18Z
======== Results ========
pass: heroku/[email protected]
fail: heroku/[email protected]
fail: heroku/[email protected]
Expected behavior

For the deprecation warning about Buildpack API version to be shown regardless of whether the buildpack in question is referenced directly, or is a transitive dependency of a composite buildpack.


Context

lifecycle version

0.17.2 (only since 0.18.x has since dropped support for Buildpack API 0.6, meaning there are no deprecated versions left at the moment with which the issue can be demonstrated).

platform version(s)

Pack 0.32.1+git-b14250b.build-5241

anything else?

The buildpacks in question:
https://github.com/heroku/buildpacks-nodejs/blob/v2.3.0/buildpacks/npm/buildpack.toml
https://github.com/heroku/buildpacks-nodejs/blob/v2.3.0/meta-buildpacks/nodejs-function/buildpack.toml

@natalieparellano
Copy link
Member

Thanks for reporting this @edmorley. On a side note, does it make sense for a composite buildpack to declare a Buildpack API version? I guess buildpack.toml has to follow some schema... but still, it feels wrong. Maybe a question for another day...

@edmorley
Copy link
Contributor Author

edmorley commented Nov 15, 2023

Good question. We need some way to version the schema, but using something like schema-version (like project.toml uses) for composite buildpacks would seem a bit strange when component buildpacks don't use schema-version and instead use the Buildpack API version to represent a combination of schema version plus implementation details version.

I suppose even though composite buildpacks don't implement detect/build, the order grouping is still quite Buildpack API spec specific -- if we ever changed how concepts like groups or optional worked, then it would seem like more than just a schema change, so at that point linking that change to an actual Buildpack API version would presumable make sense. As such, perhaps it's fine for composite buildpacks to have an API version?

@edmorley
Copy link
Contributor Author

This issue is currently in the lifecycle 0.17.3 milestone, but (a) this issue it should probably be in one of the newer milestones, (b) the lifecycle 0.17.3 milestone should probably be closed out, given that version was already released?

Also, ref this issue: I don't think adding a Buildpack API version to a composite buildpack is the correct fix. I think the deprecation check should always iterate through the dependent buildpacks - since there's would be no guarantee that the composite buildpack even uses the same Buildpack API version as it's component buildpack dependents.

@natalieparellano natalieparellano added the help wanted Need some extra hands to the this done. label Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Need some extra hands to the this done. status/ready type/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants