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

Do not enforce presence of CNB_STACK_ID #283

Merged
merged 1 commit into from
Apr 18, 2024

Conversation

c0d1ngm0nk3y
Copy link
Contributor

IfCNB_STACK_ID is not present, the buildpacks should not just fail.

related rfc: https://github.com/buildpacks/rfcs/blob/main/text/0096-remove-stacks-mixins.md
first step for fixing paketo-buildpacks/libpak#329

This also aligns the behavior with packit, so that all paketo buildpacks behave similar.

Co-authored-by: Ralf Pannemans <ralf.pannemans@sap.com>
Signed-off-by: Johannes Dillmann <j.dillmann@sap.com>
@dmikusa
Copy link
Contributor

dmikusa commented Apr 16, 2024

Can you expand on this? Where is this creating a problem? Is this part of the work we need to do to deprecate stacks?

@c0d1ngm0nk3y
Copy link
Contributor Author

Can you expand on this?

If the platform provides CNB_STACK_ID, this makes no difference. But when the platform does not provide it, it even fails, although no buildpack (that we found) relied on the presence.

Where is this creating a problem?

When using a custom stack in our platform, it works fine for nodejs (packit), but fails for java (libcnb) because we do not have a CNB_STACK_ID. The alternative would be to make up a CNB_STACK_ID or even set it to empty. But this change would align packit und libcnb a bit more.

Is this part of the work we need to do to deprecate stacks?

This would be a good first step towards removing the stack.

@dmikusa Is there any potential problem that we did not see?

@dmikusa
Copy link
Contributor

dmikusa commented Apr 18, 2024

Mostly just trying to understand the request and requirements but also a little concerned with keeping spec compatibility. That said I haven't looked to check what, if anything, the spec says for the versions we presently support.

@c0d1ngm0nk3y
Copy link
Contributor Author

... a little concerned with keeping spec compatibility.

But that would mean that packit wouldn't be spec conform :(

That said I haven't looked to check what, if anything, the spec says for the versions we presently support.

That would be good to know indeed.

@dmikusa
Copy link
Contributor

dmikusa commented Apr 18, 2024

In v1.x, we supported buildpack API 0.5 to 0.8.

The buildpack spec mentions BP_STACK_ID in one section. It is the same in all of those versions.

Buildpacks MAY use the value of CNB_STACK_ID to modify their behavior when executed on different stacks.

Looking in the platform spec it's trickier cause I don't think the two are linked, but I see some notes about this env variable being optional. See here.

The following variables SHOULD be set in the lifecycle execution environment and SHALL be directly inherited by the buildpack without modification:

and then it goes on to list CNB_STACK_ID.

It is consistent up to platform spec 0.12 where stacks are deprecated. Even then, it recommends that stack authors keep setting that and that the platform pass it through.

I also looked for places where it might be referenced and didn't see any. It looks like libcnb is just pulling it into the context so it's easily accessible to buildpacks. That said, buildpacks may use and operate differently if this is not set. For example, I know there are some Paketo buildpacks that use the stack ID to make assumptions about the contents of the runtime image.

Given that, it seems reasonable to me to make this change. The env variable isn't required, so we shouldn't fail when it's not set.

Are you planning to PR this for 1.x as well? I think we could do that safely, if you need it.

@dmikusa dmikusa added type:bug A general bug semver:patch A change requiring a patch version bump labels Apr 18, 2024
@dmikusa dmikusa merged commit 29547c4 into buildpacks:main Apr 18, 2024
6 of 8 checks passed
@c0d1ngm0nk3y
Copy link
Contributor Author

Are you planning to PR this for 1.x as well? I think we could do that safely, if you need it.

Will do. I always forget that v2 is main whereas in libpak it is the other way round. Thanks for the reminder.

@c0d1ngm0nk3y
Copy link
Contributor Author

Will do

#284

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump type:bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants