-
Notifications
You must be signed in to change notification settings - Fork 71
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: Replace positional args to Buildpack executables with env vars #190
Conversation
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
Maintainers, As you review this RFC please queue up issues to be created using the following commands:
Issues
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it - might still be worth passing in the CLI args for b/w compat but happy with either.
@samj1912 it's kind of buried, but I'm proposing something close to that:
|
I feel like this is probably going to be the quickest rfc we have put in recently then XD |
# What it is | ||
[what-it-is]: #what-it-is | ||
|
||
We will deprecate the positional arguments to `bin/detect` and `bin/build` with the following environment variables: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will deprecate the positional arguments to `bin/detect` and `bin/build` with the following environment variables: | |
We will replace the positional arguments to `bin/detect` and `bin/build` with the following environment variables: |
I feel strongly that we should just make a hard cut over between API versions. While this will require buildpack authors to preform migration steps when moving to the new API, it's the only way to keep API version complexity in check and it is consistent with our recent policies/behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I think that's true, this is a pretty big shift change. How many of these kind of changes do we want to make? I strongly feel that proposals like #172 are disruptive and fairly drastic and we can't keep going down this kind of path if we want to keep a community around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to make this change I think there are two options are
- option 1 - no deprecation requires buildpack authors to migrate to the new usage when they adopt buildpack API 0.10 (or whatever API it lands in)
- option 2 - deprecation requires buildpack authors to migrate to the new usage when they adopt buildpack API 0.17 (or whatever API it is removed in)
The way I see it the amount of total pain is the same in both cases and delaying the migration until we remove a bunch of deprecated features all at once, isn't necessarily better for buildpack authors. I would feel differently if users had to migrate to upgrade the lifecycle, but they can run the latest lifecycle and stay on the current API as long as they like (until we deprecate and remove the API).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With any breaking change, I think we need to compare the burden of supporting backwards compat with the burden we put on our users/community to migrate. In this case, the burden to migrate can be very high (I would compare it to the addition of the [types]
table in layer toml). At the same time, I don't see how continuing to support the env vars adds "complexity".
We only get to make so many breaking changes before people stop upgrading, or leave the community. I think #172 consumes our budget for that tolerance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ekcasey i'm not proposing we remove the positional args at all
cmd.Env = append(cmd.Env, EnvPlatformDir+"="+b.Dir) | ||
``` | ||
|
||
The positional arguments will be deprecated, but no warnings will be emitted if they are consumed. The lifecycle will continue to provide them to buildpack executable indefinitely, with no plan to remove them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel strongly that we should just remove them for buildpacks opting into the new API.
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
@sclevine please re-review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve conditional on platform env var changing to CNB_PARENT_LAYERS_DIR
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
Signed-off-by: Terence Lee <hone02@gmail.com>
Best on @ekcasey's comments on how we should be moving things into FCP with the votes, I'm going to move this into FCP that will close by Feb 11. (next Friday) |
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
A grievance has been raised about the env var names, their conflicts with existing env var names, and the complexity associated with renaming them. Here are some options we may consider: Option 1Use a new prefix,
Drawback: doesn't match Option 2Just reuse the var names. It's a different context and people will know the difference
Option 3Use slightly less obvious/intuitive names that are unambiguous
Drawback: this might make it difficult if we ever want to introduce new vars (we'd have to avoid collision every time) |
@jkutner Thank you kindly. I would be happy moving forward with any of these options. I think my preference order is 2, 1, 3, but I am happy to defer to others (looking at you @sclevine) who might strongly prefer one to the other. Option 2b: Just like option 2 but we could still use |
I suppose an option 4 (building on @ekcasey's 2b) is:
All of these are unique, but how long can we get away with that without a special prefix? Is now the the time to assert |
In the almost four years of the project, we've only added one new "input" ( |
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
|
||
### bin/detect | ||
|
||
* `CNB_PLATFORM_DIR` - replaces the first positional argument to `bin/build`. Uses the same env var name as the Platform spec. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/queue-issue buildpacks/lifecycle "Add CNB_PLATFORM_DIR and CNB_BUILD_PLAN_PATH to bin/detect"
|
||
### bin/build | ||
|
||
* `CNB_LAYERS_DIR` - replaces the first positional argument to `bin/build`. **Note:** Uses the same env var name as the Platform spec, but refers to a different location. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/queue-issue buildpacks/lifecycle "Add CNB_LAYERS_DIR, CNB_PLATFORM_DIR and CNB_BP_PLAN_PATH to bin/build"
Readable