heroku / cnb-shim

This is a shim to run old buildpacks as Cloud Native Buildpacks
MIT License
18 stars 12 forks source link

Buildpack API >= 0.6 requires layer to define default process type #67

Closed samatcd closed 1 year ago

samatcd commented 1 year ago

The cnb-shim was updated from buildpack API version 0.4 to version 0.8 as part of this commit: https://github.com/heroku/cnb-shim/commit/90c6e672d24f73ed6ee3b57485829b0773d5f51e

As part of the API upgrade notes from 0.5 -> 0.6, it's noted that the API will no longer set the default process type to web, and that buildpacks should contribute a default type themselves in launch.toml. https://buildpacks.io/docs/reference/spec/migration/buildpack-api-0.5-0.6/#buildpacks-contribute-default-process-type

We use GitLab auto-build-image with cnb-shim to build PHP applications using the heroku php buildpack — the shim doesn't seem to set the default process type in launch.toml and unfortunately auto-build-image doesn't allow us to pass a --default-process argument to the pack command, so we're a little stuck.

Is there any way to update the cnb-shim to set the default process?

samatcd commented 1 year ago

Workaround for now:

In project.toml change your buildpack shim uri:

[[build.buildpacks]]
uri = "https://cnb-shim.herokuapp.com/v1/heroku/php?api=0.4"

Edit: this still doesn't work because of the change to:

[types]
launch = true
edmorley commented 1 year ago

@samatcd Thank you for filing this, and sorry for missing this breaking change during #66. I hadn't realised that this buildpack even read the procfile, since in the Heroku builders we use the actual heroku/procfile CNB.

So it seems a bit unfortunate that cnb-shim duplicates the Procfile CNB: https://github.com/heroku/cnb-shim/blob/90c6e672d24f73ed6ee3b57485829b0773d5f51e/releaser.go#L25-L45 https://github.com/heroku/procfile-cnb/blob/cd7f0b49f5c907955f2275d9b7f2ad808e43409e/src/launch.rs#L5-L35

To resolve the default process type issue, we'd need to duplicate yet more of the implementation from the Procfile CNB. Whilst it wouldn't take long to do that, I wonder whether longer term we'd be better off removing cnb-shim's own procfile handling, and instead either:

  1. Document that shimmed CNBs need to be used alongside the heroku/procfile CNB (or an equivalent CNB)
  2. Or, have cnb-shim create a meta-buildpack that wraps the shimmed CNB and also adds heroku/procfile to the meta-buildpacks order?
samatcd commented 1 year ago

Ahh yes, interesting. It definitely doesn't make sense to maintain this in two places.

My initial thought would be to implement option 2: Have cnb-shim add heroku/procfile automatically, as that would be less of a breaking change.

edmorley commented 1 year ago

So looking closer, even if we removed the part of cnb-shim that duplicates the procfile CNB, we'd still need some processes handling in CNB shim, since it needs to support the default_process_types from bin/release feature of the classic buildpack API: https://devcenter.heroku.com/articles/buildpack-api#bin-release

(Which is implemented here: https://github.com/heroku/cnb-shim/blob/90c6e672d24f73ed6ee3b57485829b0773d5f51e/releaser.go#L20-L33)

Another issue I found is that cnb-shim uses libbuildpack for the Process type, and:

This all means the time investment to resolve this issue has now increased to the point where it's not something I have time to fix now, so instead will be rolling back #66 and accepting that cnb-shim will cause deprecation warnings again (when used with lifecycle 0.16+) and is using a Buildpack API version that stops being supported upstream as of July 2023.

Longer term, we'll need to decide what the future of cnb-shim should be. The codebase is in a not so great state, it doesn't have integration tests, it allows users to customise the API version but yet doesn't have the architecture to support multiple API versions at once, the server is not properly productionised etc.

One option would be for us to write a newer, simpler implementation that leverages our existing investment in libcnb.rs.

edmorley commented 1 year ago

This has been resolved by reverting the change in default buildpack API version in #68.

I've filed #69 for tracking resolving the deprecation warnings (that were the reason for the update in default Buildpack API version in the first place) longer term.

In the meantime, if anyone wants to use the newer buildpack API version (with the understanding that it's not fully supported yet, per the OP here), they can override the default by using &api=0.8 etc in the cnb-shim buildpack URL.