-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add support for buildpack api 0.8 #131
Conversation
Signed-off-by: Sambhav Kothari <skothari44@bloomberg.net>
Signed-off-by: Sambhav Kothari <skothari44@bloomberg.net>
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.
Looks good to me.
I'm not 100% sure it's necessary, but my only observation is that in the detect & build tests, we're not testing the case where both args & env variables are set.
I believe that the code is going to take the env variables if both are set. It's an invalid state, so it would seem reasonable if we errored if both are set. I don't know if the platform/lifecycle would allow this (I didn't test), but my line of thinking is that it might be possible for a user to set one of these new env variables on an older platform/lifecycle since the older platform/lifecycle is unaware of them altering the behavior. In this particular case, erroring out would be beneficial.
|
||
ctx.Platform.Path = config.arguments[2] | ||
logger.Debugf("Layers: %+v", ctx.Layers) |
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.
logger.Debugf("Layers: %+v", ctx.Layers) | |
logger.Debugf("Layers Directory: %+v", ctx.Layers) |
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.
Is it okay if we keep it to the previous statement since that is what libcnb was logging before?
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.
Oh, I thought that had changed in the diff. Yes, not a huge issue. Just thought that would be a little clearer.
Even if the user sets this on an older version of the platform/lifecycle, we only source from the env var if the buildpack api >= 0.8. |
Resolves #128