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

CNB_BUILDPACK_DIR environment variable #71

Closed

Conversation

Malax
Copy link
Contributor

@Malax Malax commented Apr 2, 2020

@Malax Malax requested a review from a team as a code owner April 2, 2020 16:31
Signed-off-by: Manuel Fuchs <malax@malax.de>
@Malax Malax force-pushed the cnb-buildpack-directory-env-var branch from 1387b17 to d050b57 Compare April 2, 2020 16:31
Copy link
Member

@hone hone left a comment

Choose a reason for hiding this comment

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

This is something I've seen a bunch of buildpacks do at Heroku. I'm curious of on the VMware/Pivotal side this is as common?

@sclevine
Copy link
Member

sclevine commented Apr 2, 2020

This is also common on the CF side. My only request before approving would be that the env var be CNB_BUILDPACK_DIR to match project conventions.

We also may want to consider why an env var might be appropriate here vs. a positional arg like the other input directories.

@Malax Malax changed the title CNB_BUILDPACK_DIRECTORY environment variable CNB_BUILDPACK_DIR environment variable Apr 3, 2020
Signed-off-by: Manuel Fuchs <malax@malax.de>
@Malax Malax force-pushed the cnb-buildpack-directory-env-var branch from 166bd7f to 1211997 Compare April 3, 2020 08:30
@Malax
Copy link
Contributor Author

Malax commented Apr 3, 2020

@sclevine I renamed the variable, thanks! :)

We also may want to consider why an env var might be appropriate here vs. a positional arg like the other input directories.

I touched on that a little bit in the RFC, assuming we want to keep the amount of positional arguments low. It's harder to remember the order of argument than an actual name. That would be true for all other arguments though. What was the deciding factor to make CNB_STACK an env var and not an argument?

@nebhale nebhale requested review from nebhale and sclevine April 9, 2020 18:18
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.

I'm going to abstain from this RFC.

@hone
Copy link
Member

hone commented Apr 9, 2020

We should make the RFC decision here independent of the positional argument/env var discussion IMO. Cleaning that up should be a separate RFC.

@sclevine
Copy link
Member

In FCP as of 4/15

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.

6 participants