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

Make Procfile validation more strict #73

Open
edmorley opened this issue Jun 17, 2022 · 4 comments
Open

Make Procfile validation more strict #73

edmorley opened this issue Jun 17, 2022 · 4 comments

Comments

@edmorley
Copy link
Member

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
Copy link
Member Author

cc @schneems @Malax

@Malax
Copy link
Member

Malax commented Jun 20, 2022

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:

<process type>: <command>

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
Copy link
Member Author

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

@schneems
Copy link
Contributor

The challenge is detection. I think we're best off moving away from regex and towards nom or winnow and we should "parse not validate." This would allow us to handle:

  • An all empty Procfile
  • A procfile with only comments (effectively empty

For the rest of the cases being able to enumerate "Here's the problem" to point at a specific line and/or character would be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants