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 #0109] pack builder create should accept builder env config #1632

Closed
1 task done
Tracked by #248
natalieparellano opened this issue Feb 13, 2023 · 14 comments · Fixed by #1926
Closed
1 task done
Tracked by #248

[RFC #0109] pack builder create should accept builder env config #1632

natalieparellano opened this issue Feb 13, 2023 · 14 comments · Fixed by #1926
Labels
good for mentorship A good issue for a mentorship project. status/ready Issue ready to be worked on. type/enhancement Issue that requests a new feature or improvement.
Milestone

Comments

@natalieparellano
Copy link
Member

natalieparellano commented Feb 13, 2023

Description

As per RFC 0109, files in /cnb/build-config/env will be translated to buildpack environment variables by the lifecycle.

It would be nice if builder.toml exposed a way to create these files.

Proposed solution

Add a table to builder.toml:

[[build.env]]
name = "<string>"
value = "<string>"
suffix = "<string>"
delim = "<string>"

that will be translated to files in /cnb/build-config/env by pack during pack builder create.

Describe alternatives you've considered

[[build.env]] mirrors [[io.buildpacks.build.env]] in project descriptor but we might also call it [[env]]

Proposed changes to the platform spec would introduce [[run.image]] to builder.toml.

Additional context

  • This feature should be documented somewhere

This may benefit from a sub-team RFC.

cc @samj1912 @AidanDelaney

@natalieparellano natalieparellano added type/enhancement Issue that requests a new feature or improvement. status/triage Issue or PR that requires contributor attention. status/ready Issue ready to be worked on. status/discussion-needed Issue or PR that requires in-depth discussion. good first issue A good first issue to get started with. good for mentorship A good issue for a mentorship project. and removed status/triage Issue or PR that requires contributor attention. status/ready Issue ready to be worked on. labels Feb 13, 2023
@natalieparellano
Copy link
Member Author

Tagging as "good first issue" as the implementation will borrow heavily from what already exists for translating [[io.buildpacks.build.env]] in project descriptor to files in <platform>/env. But this will probably require at least some mentorship from an experienced contributor to make this pleasant for someone who is totally new to the project.

@bhavyastar
Copy link

Hey @natalieparellano, I want to work on this issue. I am also looking to participate in LFX Mentorship so it will be a good mentorship issue for me so that I will also get familiar with the project. Can you please assign me?

@jjbustamante
Copy link
Member

@bhavyastar I just assigned to you #1615, do you prefer to work on that one first?

@natalieparellano natalieparellano removed the good first issue A good first issue to get started with. label Feb 14, 2023
@natalieparellano
Copy link
Member Author

natalieparellano commented Feb 14, 2023

Apologies, this issue is actually blocked on the discussion that will establish the desired schema for builder.toml (and whether an RFC is necessary). Hopefully this could be sorted soon, in the meantime it's probably best to start in on another issue :)

@jjbustamante
Copy link
Member

We still need to discuss about the schema for builder.toml should look it, let's bring this up in the next working group meeting

@jjbustamante
Copy link
Member

@natalieparellano natalieparellano added this to the 0.32.0 milestone Aug 25, 2023
@jjbustamante jjbustamante added status/ready Issue ready to be worked on. and removed status/discussion-needed Issue or PR that requires in-depth discussion. labels Sep 7, 2023
@jjbustamante
Copy link
Member

We talked about this during the working group meeting today, the proposed solution looks good for everybody so, I will mark this ticket as ready to be implemented

@natalieparellano
Copy link
Member Author

Updated [[build.env]] schema to include suffix & delimiter.

@WYGIN
Copy link
Contributor

WYGIN commented Oct 4, 2023

i am interested in contributing to this issue & i have gone through the links provided above, but still i have left with few doubts

  • allow environment variables using the build image has done with the lifecycle part & reading files in /cnb/build-config/env
    from the tests & code of the above pr i understood that the files in pr should be a full stop(.) delimited filenames where
  • prefix in filename is the key/name of the env
  • suffix of the filename is one of [override/default/append/prepend/delim]
  • the content of the file is the value of key/name of the env

Add a table to builder.toml:

[[build.env]]
name = "<string>"
value = "<string>"
suffix = "<string>"
delim = "<string>"
  • if i am not wrong, the [[build.env]] can be defined multiple times, where each [[build.env]] will generate a seperate file in /cnb/build-config/env based on name, suffix fields

that will be translated to files in /cnb/build-config/env by pack during pack builder create.

from the above quote i need to create files in dir /cnb/build-config/env with the above filename conventions during pack builder create. but i have few doubts regarding it

  • should the [[build.env]] can be defined through a flag from cli with pack builder create ?
  • if pack builder create accepts --config that has [[build.env]] then what is the use of delim = "<string>", looks like lifecycle accepts prefix = "delim" to perform same behavior, how prefix = "delim" & delim = "<string>" differ each other
  • if [[build.env]] ids passed through cli what should be the format of the input should be from cli ?
  • if i create the files in dir /cnb/build-config/env based on cli flag input such as --build-env, what if the content of the toml config file's [[build.env]] changed by end user ?
  • from this project.toml pr the build.go file is parsing the toml file & generating env based on the content & passing it to packBuild.Build but in this pr case the build envs defined should generate files in /cnb/build-config/env
  • looks like the [[build.env]] table is already exists in toml file & new fields suffix & delim are added to existing table, so are these two fields required ?
  • if suffix & delim are optional then should i need to pass env through project.toml build.go if those fields are not defined & if those fields defined then i need to create files in dir /cnb/build-config/env instead of passing those values to build.go's packBuild.Build ?

@natalieparellano
Copy link
Member Author

@WYGIN thank you for taking such a careful look at this! I will try to answer your questions below. I hope this is helpful - let us know if anything else is unclear.

should the [[build.env]] can be defined through a flag from cli with pack builder create ?

For now, I don't think so. This aligns with how we handle other comparable config values (such as base image name).

if pack builder create accepts --config that has [[build.env]] then what is the use of delim = "", looks like lifecycle accepts prefix = "delim" to perform same behavior, how prefix = "delim" & delim = "" differ each other

I apologize for the vague requirements here. If I have this config:

[[build.env]]
name = "MY_VAR"
value = "my-val"
suffix = "prepend"
delim = ":"

Then I should get this file structure:

/cnb/build-config/env/MY_VAR.prepend with contents my-val

/cnb/build-config/env/MY_VAR.delim with contents :

from this project.toml pr the build.go file is parsing the toml file & generating env based on the content & passing it to packBuild.Build but in this pr case the build envs defined should generate files in /cnb/build-config/env

Correct - project.toml env vars go in /platform/env and builder.toml env vars go in /cnb/build-config/env - but fundamentally they are doing the same thing. However it's worth noting that project.toml env vars don't have support for suffix or delim.

looks like the [[build.env]] table is already exists in toml file & new fields suffix & delim are added to existing table, so are these two fields required ?

[[build.env]] in project.toml should remain unchanged. This issue would add [[build.env]] to builder.toml.

if suffix & delim are optional then should i need to pass env through project.toml build.go if those fields are not defined & if those fields defined then i need to create files in dir /cnb/build-config/env instead of passing those values to build.go's packBuild.Build ?

suffix or delim (while undefined in project.toml) are indeed optional in builder.toml - missing suffix means file would have no suffix (so just /cnb/build-config/env/MY_VAR) and missing delim means we would not create a /cnb/build-config/env/MY_VAR.delim file.

@WYGIN
Copy link
Contributor

WYGIN commented Oct 5, 2023

[[build.env]]
name = "MY_VAR"
value = "my-val"
suffix = "prepend"
delim = ":"

Then I should get this file structure:
/cnb/build-config/env/MY_VAR.prepend with contents my-val
/cnb/build-config/env/MY_VAR.delim with contents :

i am thinking about having builder.toml's [[build.env]] to have the following structure

[[build.env]]
name = "MY_VAR"
value = "my-value"
action = "prepend"
[[build.env]]
name = "MY_VAR"
value = ":"
action = "delim"

because

  • the end-user will only define delim if he/she uses prepend or append
    when parsing builder.toml file it will cause unnecessary complexity such as

  • the parser should need to check for every single [[build.env]]'s name field to check if the delim is provided or not
    if the end-user provides the following builder.toml and should override delim with the last read delim value

[[build.env]]
name = "MY_VAR"
value = "prependValue"
suffix = "prepend"
delim = ":"
[[build.env]]
name = "MY_VAR"
value = "defaultValue"
suffix = "default"
[[build.env]]
name = "MY_VAR"
value = "appendValue"
suffix = "append"
delim = ";"

the user expects the result should be MY_VAR=prepend:defaultValue;appendValue but we cannot creating multiple files with name MY_VAR.delim with contents : and ;, so we either create MY_VAR=prepend:defaultValue:appendValue or MY_VAR=prepend;defaultValue;appendValue or throw an error when parsing builder.toml
this might cause the end-user to get confused by the output

apart from these i have the following doubts

  • should i need to throw an error/warning or should i ignore if the user provides name in lower/camel/kebab case instead of upper case
  • what if the end-user wants to append or prepend multiple values instead of a single value ? as it is not even implemented in the pr test or in env
    i am thinking that if the above case is possible then the user will provide value field with value having delim seperated values, but we can also accept an array of value to simplify user experience & create the content of the file with delim seperated values where the file is not encoded to escape special characters in case if it is implemented

@natalieparellano
Copy link
Member Author

natalieparellano commented Oct 5, 2023

Ah, I don't think we ever expect MY_VAR to be defined multiple times. The suffix & delim indicate how these operator-defined variables should be combined with user-provided (via <platform>/env) or buildpack-provided values, but we don't really expect them to be combined with each other, if that makes sense. If there are multiple definitions of the same var in builder.toml, we could prefer the last value but warn that we're overriding something that was previously defined.

That said I'd be in favor of keeping the simpler syntax below, as it's less to type and also avoids introducing new concept/terminology action that users have to understand.

[[build.env]]
name = "MY_VAR"
value = "my-val"
suffix = "prepend"
delim = ":"

should i need to throw an error/warning or should i ignore if the user provides name in lower/camel/kebab case instead of upper case

Nope, in fact lowercase names are perfectly valid :) We should just respect whatever is defined.

what if the end-user wants to append or prepend multiple values instead of a single value

I guess they would do something like value = "my-val1:my-val2" delim = ":" but that's a bit of an edge case - I don't think we need to worry too much about it. Let's keep the value as a string as that will be more intuitive for most users.

@WYGIN
Copy link
Contributor

WYGIN commented Oct 6, 2023

i created logic to add a layer to image with buildConfigEnv files in /cnb/build-config/env/ folder and i am trying to look into passing buildConfigDir to lifecycle (i will try to solve it by myself, & looks like it is read from env, but still i want to confirm it), but how can i assure that files in /cnb/build-config/env/ are updating the Buildpack detect and build environment based on the directory ? i want to confirm everything working perfectly on my local machine
i tried to pack buildpack inspect <image-name> [flags] & pack builder inspect <builder-image-name> [flags] & pack inspect <image-name> [flags] on sample images but i haven't got any info about ENVIRONMENT variables & does imgutil.Image#Env can be used to assert it in tests ?

@natalieparellano
Copy link
Member Author

how can i assure that files in /cnb/build-config/env/ are updating the Buildpack detect and build environment based on the directory

If you want to see this working end-to-end you could create a buildpack that prints out its environment. Then set [[build.env]] in your builder.toml and pack build some-app -B test-builder --buildpack test-buildpack and hopefully you see the env print out :) The lifecycle version should be latest or close to latest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good for mentorship A good issue for a mentorship project. status/ready Issue ready to be worked on. type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants