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

RFC: Pack Build Default Process Flag #28

Merged
merged 4 commits into from
Dec 19, 2019

Conversation

hone
Copy link
Member

@hone hone commented Oct 29, 2019

@hone hone changed the title Pack Build Process Flag RFC draft RFC: Pack Build Process Flag RFC draft Oct 29, 2019
@hone hone requested a review from jkutner October 29, 2019 23:14
@sclevine
Copy link
Member

Would we prefer the --process flag set CMD or set CNB_PROCESS_TYPE?
(The spec suggests using CNB_PROCESS_TYPE.)

@hone hone requested review from nebhale and sclevine October 30, 2019 19:06
@hone
Copy link
Member Author

hone commented Oct 30, 2019

@sclevine that would just set the env var?

@sclevine
Copy link
Member

Yep, on the image so that launcher can read it.

@jkutner
Copy link
Member

jkutner commented Oct 30, 2019

I prefer setting CMD as it aligns better with standard OCI image stuff (which seems like something we should try to do when possible). For example, a consumer of the image would be able to easily understand why it's running the worker process instead of the web process without knowing anything about CNB_ vars.

@sclevine sclevine requested a review from ekcasey October 31, 2019 18:25
Copy link
Contributor

@nebhale nebhale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy with the CMD strategy. Expecting another RFC to determine precedence between CMD, $CNB_PROCESS_TYPE, and other possibilities.

@hone hone changed the title RFC: Pack Build Process Flag RFC draft RFC: Pack Build Process Flag Nov 6, 2019
@ekcasey
Copy link
Member

ekcasey commented Nov 6, 2019

I think CNB_PROCESS_TYPE is a better path forward. That means we are changing the default process type, but a user can still override by explicitly overriding CNB_PROCESS_TYPE. That override wouldn't work if we set CMD instead, given the current precedence. I don't see a compelling reason to change the precedence.

@ekcasey
Copy link
Member

ekcasey commented Nov 6, 2019

I wonder if we could have two flags, --default-process <process-type> which sets CNB_PROCESS_TYPE and another --process <type>="<command>" which creates an entry in metadata.toml that looks like: (assume direct false, and no args)

[[processes]]
type = "<type>"
command = "<command>"
args = []
direct = false

You could also support --process=</path/to/process.toml> where process.toml supports all process options.

To add a process and make it the default you would need both --process and --default-process flags. Or you could use --process to override web and it would be the default.

@hone hone requested a review from jkutner November 7, 2019 22:40
Signed-off-by: Terence Lee <hone02@gmail.com>
Signed-off-by: Terence Lee <hone02@gmail.com>
Signed-off-by: Terence Lee <hone02@gmail.com>
@hone
Copy link
Member Author

hone commented Nov 14, 2019

@nebhale @sclevine @jkutner updated the PR based off of @ekcasey's comments on our discussion. Can you please re-review.

@hone
Copy link
Member Author

hone commented Dec 6, 2019

With @jkutner's vote this has entered Final Comment Period. This will be open until next Thurs on which it will be merged and accepted at the WG meeting.

@hone hone changed the title RFC: Pack Build Process Flag RFC: Pack Build Default Process Flag Dec 6, 2019
Signed-off-by: Terence Lee <hone02@gmail.com>
@jkutner jkutner merged commit 8e3e756 into buildpacks:master Dec 19, 2019
nebhale added a commit that referenced this pull request Feb 24, 2020
[resolves #48]

Signed-off-by: Ben Hale <bhale@pivotal.io>
zmackie pushed a commit to zmackie/rfcs that referenced this pull request Mar 30, 2020
[resolves buildpacks#48]

Signed-off-by: Ben Hale <bhale@pivotal.io>
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

Successfully merging this pull request may close these issues.

5 participants