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.

Sign up for GitHub

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

Jump to bottom

Define CNB_BUILD_CONFIG_DIR behavior #345

Merged
merged 7 commits into from
Feb 7, 2023
Merged

Define CNB_BUILD_CONFIG_DIR behavior #345

merged 7 commits into from
Feb 7, 2023

Conversation

AidanDelaney
Copy link
Member

@AidanDelaney AidanDelaney commented Dec 19, 2022 •
edited by samj1912

RFC #109 defines a directory allowing operators to set environment variables for detect and build phases. Specify how the buildpack phases should implement the behavior.

Fixes #330

Signed-off-by: Aidan Delaney adelaney21@bloomberg.net

edmorley and others added 2 commits December 4, 2022 13:45
@edmorley
Fix schema version markdown rendering in project-descriptor.md
2bf99f1 
The `<...>` references must be wrapped with backticks, or they are not rendered at all, making the spec hard to follow.

Example of the current broken rendering:

> MUST be in form . or , where is equivalent to .0



Signed-off-by: Ed Morley <501702+edmorley@users.noreply.github.com>
@samj1912
Merge pull request #334 from edmorley/patch-1
53d6f3d 
Fix schema version markdown rendering in project-descriptor.md
@AidanDelaney AidanDelaney requested a review from a team as a code owner December 19, 2022 07:42
@AidanDelaney AidanDelaney mentioned this pull request Dec 19, 2022
Open
7 tasks
samj1912
samj1912 reviewed Dec 19, 2022
View reviewed changes
buildpack.md Outdated Show resolved Hide resolved
samj1912
samj1912 reviewed Dec 20, 2022
View reviewed changes
Copy link
Member

@samj1912 samj1912 left a comment

Choose a reason for hiding this comment

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

Left some comments in the Buildpacks API about sections that don't belong there. Separately we need to target the above changes in platform 0.11 branch.

buildpack.md Outdated Show resolved Hide resolved
buildpack.md Outdated Show resolved Hide resolved
natalieparellano
natalieparellano reviewed Jan 4, 2023
View reviewed changes
buildpack.md Outdated Show resolved Hide resolved
buildpack.md Outdated Show resolved Hide resolved
natalieparellano
natalieparellano reviewed Jan 4, 2023
View reviewed changes
buildpack.md Outdated Show resolved Hide resolved
natalieparellano
natalieparellano reviewed Jan 4, 2023
View reviewed changes
buildpack.md Outdated Show resolved Hide resolved
natalieparellano
natalieparellano reviewed Jan 4, 2023
View reviewed changes
platform.md Outdated Show resolved Hide resolved
natalieparellano
natalieparellano reviewed Jan 4, 2023
View reviewed changes
platform.md Outdated Show resolved Hide resolved
natalieparellano
natalieparellano reviewed Jan 4, 2023
View reviewed changes
platform.md Outdated Show resolved Hide resolved
natalieparellano
natalieparellano reviewed Jan 4, 2023
View reviewed changes
platform.md Outdated Show resolved Hide resolved
natalieparellano
natalieparellano reviewed Jan 4, 2023
View reviewed changes
Copy link
Member

@natalieparellano natalieparellano left a comment

Choose a reason for hiding this comment

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

We should also add something at or near https://github.com/buildpacks/spec/blob/main/platform.md#user-provided-variables describing (to the operator) how the env var files are supposed to work. Something like https://github.com/buildpacks/spec/blob/main/buildpack.md#environment-variable-modification-rules from the buildpack API, but tailored to the build config dir.

@natalieparellano
Copy link
Member

natalieparellano commented Jan 5, 2023 •
edited

Related Slack conversation: https://cloud-native.slack.com/archives/C033DV9EBDF/p1672938165893439

I know this is late in the game, but to my eye changing the default behavior (when files are suffix-less) to override would make this so much better! At least, it would be more consistent with the way other env directories are applied. I was bit by this when validating the implementation because I assumed the build config env dir would work like buildpack env dirs and they do not...

Edit: concluded in Slack, default makes more sense as a default at the builder level because we don't want to clobber user provided vars unless we really intend to.

@AidanDelaney
Copy link
Member Author

We should also add something at or near https://github.com/buildpacks/spec/blob/main/platform.md#user-provided-variables describing (to the operator) how the env var files are supposed to work. Something like https://github.com/buildpacks/spec/blob/main/buildpack.md#environment-variable-modification-rules from the buildpack API, but tailored to the build config dir.

I've not yet addressed this comment. Will do so when buildpacks/rfcs#271 lands, as that will confirm my understanding.

@natalieparellano natalieparellano mentioned this pull request Jan 6, 2023
Merged
@AidanDelaney
Define CNB_BUILD_CONFIG_DIR behavior
2c5a805 
RFC #109 defines a directory allowing operators to set environment
variables for detect and build phases.  Specify how the buildpack
phases should implement the behavior.

Signed-off-by: Aidan Delaney <adelaney21@bloomberg.net>
@natalieparellano
Copy link
Member

@AidanDelaney were you planning to add:

We should also add something at or near https://github.com/buildpacks/spec/blob/main/platform.md#user-provided-variables describing (to the operator) how the env var files are supposed to work. Something like https://github.com/buildpacks/spec/blob/main/buildpack.md#environment-variable-modification-rules from the buildpack API, but tailored to the build config dir.

samj1912
samj1912 previously approved these changes Jan 31, 2023
View reviewed changes
samj1912
samj1912 requested changes Jan 31, 2023
View reviewed changes
Copy link
Member

@samj1912 samj1912 left a comment

Choose a reason for hiding this comment

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

We need to split this PR into platform and buildpack api changes and set the base branch to the relevant target apis.

@AidanDelaney
Copy link
Member Author

I've split the buildpack.md changes out into #349

@AidanDelaney
Describe Operator provided vars in CNB_BUILD_CONFIG_DIR
f4424e9 
Describe how CNB_BUILD_CONFIG_DIR variables MUST be provided in the
lifecycle execution environment and the suffixes that are allowed.

Signed-off-by: Aidan Delaney <adelaney21@bloomberg.net>
@AidanDelaney
Provide changes to platform.md only
4cff1db 
Revert changes to buildpack.md so that platform.md can be merged into
appropriate branches.

Signed-off-by: Aidan Delaney <adelaney21@bloomberg.net>
@AidanDelaney
Copy link
Member Author

This PR now only contains the changes to platform.md. @natalieparellano I've added a section on "Operator Defined Environment Variables".

@samj1912 samj1912 changed the base branch from main to platform/0.11 February 4, 2023 13:46
samj1912
samj1912 reviewed Feb 4, 2023
View reviewed changes
platform.md Outdated Show resolved Hide resolved
samj1912
samj1912 reviewed Feb 4, 2023
View reviewed changes
platform.md Outdated Show resolved Hide resolved
@samj1912
Apply suggestions from code review
4eb5f02 
Signed-off-by: Sambhav Kothari <sambhavs.email@gmail.com>
samj1912
samj1912 reviewed Feb 4, 2023
View reviewed changes
platform.md Outdated

The platform MUST set operator-provided environment variables directly in the lifecycle execution environment.

The `CNB_BUILD_CONFIG_DIR` directory follows the same convention as [Environment Variable Modification Rules](https://github.com/buildpacks/spec/blob/main/buildpack.md#environment-variable-modification-rules).
Copy link
Member

Choose a reason for hiding this comment

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

Can we add that if no extension is provided, the default extension is .default?

Copy link
Member

Choose a reason for hiding this comment

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

Added a suggestion

natalieparellano
natalieparellano reviewed Feb 7, 2023
View reviewed changes
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
@samj1912 @natalieparellano
Apply suggestions from code review
f074ff2 
Co-authored-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Sambhav Kothari <sambhavs.email@gmail.com>
@samj1912 samj1912 merged commit a79c895 into platform/0.11 Feb 7, 2023
@samj1912 samj1912 deleted the rfc-109-env-build branch February 7, 2023 23:13
@natalieparellano natalieparellano mentioned this pull request Feb 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@natalieparellano natalieparellano natalieparellano left review comments

@samj1912 samj1912 samj1912 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RFC #0109] Allow setting environment variables using the build image
4 participants
@AidanDelaney @natalieparellano @samj1912 @edmorley