search

heroku / buildpacks-procfile

Heroku's Procfile Cloud Native Buildpack.
BSD 3-Clause "New" or "Revised" License
0 stars 1 forks source link

Make `Procfile` validation more strict #73

Open edmorley opened 2 years ago

edmorley commented 2 years ago

Both this buildpack and Codon will happily accept a Procfile with contents:

web

And will set the command to the empty string.

Should we make both this buildpack and Codon reject this?

See: https://github.com/heroku/procfile-cnb/blob/683de9b9877957ca0a5e6ba6733059cb55b5c4d9/src/procfile.rs#L41-L61

edmorley commented 2 years ago

cc @schneems @Malax

Malax commented 2 years ago

Seems a bit broader in scope than this buildpack, heck - it's great to see that this case is handled in a compatible way by this buildpack. :)

To the actual issue, a Procfile with just web in it is nonsensical to me. I cannot imagine we're breaking actual app builds when we start rejecting these Procfiles.

The "spec" on DevCenter states that the following is the format:

\: \

To me, that makes the colon mandatory. I'm +1 on changing this behaviour across the platform. However, I don't think this carries any urgency at this point.

edmorley commented 1 year ago

This ticket is an example of where the lax parsing (that non-CNB Heroku also uses) causes poor UX: https://heroku.support/1306718

In that case the Procfile contained contents like:

heroku ps:scale web=N

And the failure mode seen by the user was at runtime:

<TIMESTAMP> app[heroku.1]: bash: line 1: ps:scale: command not found