Replace regex with winnow based parser

[schneems]

Replace the existing Regex based "parser" with an actual honest-to-goodness parser/combinator based parser.

• Close Enforce RFC 1123 on procfile process types #251
• Close Stop using regex for validation #178
• Close Make Procfile validation more strict #73

How to review

Some suggestions on how to review this:

• Proofread/comment on SPEC.md in the fist commit. This is the basis for the rest of the parser work so if you disagree with something there, it's not worth continuing on into the implementation
• Read the changes to the integration tests on the final result: Warnings have been added, but otherwise assert that it's a refactor.
• Read the added parsing error message in error.rs
• Read the unit tests in procfile.rs. Some of them were useful for development, the majority are asserting overall behavior and edge cases
• If you want to review the actual winnow and parser code, please book a pairing session with me. It's a lot and is better to talk through syncronously. Feel free to suggest any documentation typos etc.

[joshwlewis]

I like that this exists, but I wonder if this spec should be shared/enforced more widely?

[schneems]

shared

You mean you're wanting more feedback from a diverse group of people (such as other Heroku teams) or you're wanting to promote the format to a larger group outside of Heroku such as language communities (or both)?

I'm open to both. I'm also open to suggestions. Perhaps we link to the SPEC from the readme and make it more prominent.

enforced

We could share the logic by providing a stand-alone parser utility that converts it to some other format like json or toml. We could start by calling both parsers in cedar and instrumenting failures. While people are using # like comments in today's procfile, technically any leading character would work //.

I also had the idea that I could expose the parser or ship it via crates.io so that other buildpacks could use it. Some buildpacks have logic like "If app has a Procfile..." but it would be more robust to share the same parsing logic.

Getting a little ahead of myself. We could also consider upstreaming RFC 1123 to the CNB spec to make it more clear so then it's not "the procfile buildpack supports the intersection of kubernetes and CNB" and then becomes simply "the procfile buildpack supports the CNB spec".

[joshwlewis]

Oh, I was just thinking share/enforce this across Heroku more broadly. This will be the third Procfile parser at Heroku, and I doubt they are consistent. This isn't a blocker for this PR though.

I'd support adjusting the CNB spec to have process names that match RFC 1123 if you wanted to do that. I expect many / most CNB users expect to use apps built with CNBs in kubernetes in some way.

[joshwlewis]

I don't think this spec covers a space before the :? Like my-process : bin/blah. I think that should be legal, but I could be wrong.

[schneems]

I think that should be legal, but I could be wrong.

Looking at the prior regex

^[[:space:]]*([a-zA-Z0-9_-]+):?\\s+(.*)[[:space:]]*

• starts with one or more whitespace characters
• alphanumeric or _ or -
• funny enough the : is optional, but it doesn't support spaces before :
• A space after the (optional) : is enforced (which is a requirement we've loosened up). It cannot be a tab, it must be a space (which we've loosened up).
• Then it grabs the value
• Then the regex makes it look like it strips off any trailing spaces but it doesn't. Rust won't strip it out:

Nor will ruby:

irb(main):005:0> match = "web: rails s         ".match(/^[[:space:]]*([a-zA-Z0-9_-]+):?\s+(.*)[[:space:]]*/);
=> #<MatchData "web: rails s         " 1:"web" 2:"rails s         ">
irb(main):006:0> match[2]
=> "rails s         "

So a space before the : wouldn't work today, but instead of an error or warning you'll quietly get an ignored line. I don't think we want to support optional :.

[edmorley]

I'm also in favour of making the spec (and the Procfile CNB implementation) more strict than the current implementation in cases where we think it makes sense. So long as the Procfile CNB error message is clear, users can tweak their Procfile if needed as part of migrating to CNB.

[Malax]

+1 on @edmorley's comment. I imagine most of the allowed weird Procfiles on cedar are accidentally valid.

[joshwlewis]

This probably warrants a major version bump. Looks good to me!