pack builder create should accept builder env config
#1926
Conversation
Signed-off-by: WYGIN <wygininc@gmail.com>
github-actions
bot
added
the
type/enhancement
WYGIN
changed the title
pack builder create should accept builder env config
Signed-off-by: WYGIN <wygininc@gmail.com>
Signed-off-by: WYGIN <wygininc@gmail.com>
Signed-off-by: WYGIN <wygininc@gmail.com>
Signed-off-by: WYGIN <wygininc@gmail.com>
|
@natalieparellano @jjbustamante i am getting following response when i use to the however i am getting following result when i run how can i check the envs of the image ? |
|
should i open a new issue in imgutil and a pr to resolve it if it is related to it |
|
Hi @WYGIN, there are several questions here. Let me try to go through them.
If you are saving the image in your docker daemon, you can use For example, having docker inspect <your-iamge> jq '.[] | .Config | .Env 'Compiling your branch and running the sample builder you mentioned, I can see: The config file for the image builder has the following environment variables > docker inspect my-builder-env:jammy | jq '.[] | .Config | .Env '
[
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
"CNB_USER_ID=1000",
"CNB_GROUP_ID=1000",
"CNB_STACK_ID=io.buildpacks.samples.stacks.jammy"
]Inspecting the image layers with dive tool:
I can see a file |
I think I found your problem.
if len(b.env) > 0 {
logger.Debugf("Provided Environment Variables\n %s", style.Map(b.env, " ", "\n"))
}
envTar, err := b.envLayer(tmpDir, b.env)
if err != nil {
return err
}Which I think is overriding the path in the layerMap because the Something like this: if len(b.buildConfigEnv) > 0 {
logger.Debugf("Provided Build Config Environment Variables\n %s", style.Map(b.env, " ", "\n"))
buildConfigEnvTar, err := b.buildConfigEnvLayer(tmpDir, b.buildConfigEnv)
if err != nil {
return errors.Wrap(err, "retrieving build-config-env layer")
}
if err := b.image.AddLayer(buildConfigEnvTar); err != nil {
return errors.Wrap(err, "adding build-config-env layer")
}
}Which I think it better, I don't know why we add the empty |
Signed-off-by: WYGIN <wygininc@gmail.com>
|
when i run the following command declare -x CNB_BP_PLAN_PATH="/tmp/samples_hello-moon-3054511201/samples_hello-moon/plan.toml"
[builder] declare -x CNB_BUILDPACK_DIR="/cnb/buildpacks/samples_hello-moon/0.0.1"
[builder] declare -x CNB_DUP_ID="9999"
[builder] declare -x CNB_LAYERS_DIR="/layers/samples_hello-moon"
[builder] declare -x CNB_PLATFORM_DIR="/platform"
[builder] declare -x CNB_STACK_ID="io.buildpacks.samples.stacks.jammy"
[builder] declare -x CNB_TARGET_ARCH="amd64"
[builder] declare -x CNB_TARGET_ARCH_VARIANT=""
[builder] declare -x CNB_TARGET_DISTRO_NAME=""
[builder] declare -x CNB_TARGET_DISTRO_VERSION=""
[builder] declare -x CNB_TARGET_OS="linux"
[builder] declare -x HI="hi"
[builder] declare -x HOME="/home/cnb"
[builder] declare -x HOSTNAME="000fe4967dab"
[builder] declare -x OLDPWD
[builder] declare -x PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
[builder] declare -x PWD="/workspace"
[builder] declare -x SHLVL="1"this is the output when i run 'env' in container's HOSTNAME=a29123dbe762
PWD=/workspace
_=/usr/bin/env
CNB_GROUP_ID=1000
HOME=/home/cnb
TERM=xterm
SHLVL=0
CNB_USER_ID=1000
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
OLDPWD=/workspaceand AFAIK docker/podman will read config-file/metadata of the container before it starts the container & it looks like it also reads envs too from the metadata & there are only few envs are getting declared in metadata & even flag |
There was a problem hiding this comment.
@WYGIN this is awesome! My apologies for the delay in getting back to you about this. The PR looks great! Thank you for testing everything so thoroughly 🙌🏼 🙏🏼
I left a couple of comments, but this is really close!
| @@ -1257,3 +1305,11 @@ func (e errModuleTar) Info() dist.ModuleInfo { | |||
| func (e errModuleTar) Path() string { | |||
| return "" | |||
| } | |||
|
|
|||
| func cnbBuildConfigDir() string { | |||
There was a problem hiding this comment.
IIUC this allows the pack user to specify the location of the build config directory within the builder by setting an environment variable in pack's environment. We might want to add some text to pack builder create --help so that this option is discoverable. Or maybe we could add it as a field in builder.toml ...or just leave it non-configurable. WDYT @WYGIN @jjbustamante?
There was a problem hiding this comment.
i am not sure about adding pack builder create --help, IMO the pack builder create --help is about giving better context to user about the list of commands available for the end-user to use, and i think CNB_BUILD_CONFIG_DIR in cli help might not required
so i added it to the docs where builder.toml fields and tables are specified
Signed-off-by: WYGIN <wygininc@gmail.com>
Signed-off-by: WYGIN <wygininc@gmail.com>
|
@natalieparellano @jjbustamante i have noticed the following differences in between operator-defined variables and environment-variable-modification-rules where
and
when the [[build.env]]'s |
Signed-off-by: WYGIN <wygininc@gmail.com>
Ah, that is okay - the lifecycle will handle that. We just need to create the files in the correct format. |
|
I think this is ready for your review @jjbustamante |
|
how can i contribute to this issue, i have never co-authored before & now i need make contribution based on changes made by husni-faiz, it would be helpful if you point me to some resource or by giving some suggestion on how to make changes made pr, if i clone husni-faiz repo i need to commit changes to his repo and he need to create a commit for my changes to reflect at buildpack, is there any simple way this @natalieparellano @jjbustamante |
I'll let @jjbustamante speak further to this as he's leading this track of work, but I think a first step would be to go through this RFC. The RFC mentions:
I believe @husni-faiz has been working on some (though perhaps not all) of the issues that relate to phase 1 (Husni could you advise if there any issues unclaimed or that you'd like help with?). We haven't started work on phase 2, mostly because the requirements are still evolving, but it might not hurt to get some idea of what issues we might need to create for phase 2 (how the work will be broken down). I am sure that any of us who've been involved with this effort would be happy to provide guidance when it comes to the implementation. |
Signed-off-by: Juan Bustamante <jbustamante@vmware.com>
|
Hi @WYGIN
As @natalieparellano mentioned, @husni-faiz is being working on this. He is struggling adding test coverage to his imgutil and pack PR. I have the following approach to contribute to Husni pull requests using my pack personal
I think in this way you can help Husni, we will really appreciate it! ❤️ , our main goal is to improve test coverage in both PR (imgutil and pack) |
|
@jjbustamante i have been deep diving into gccr, podman's image, buildah, storage and other repos and also docker cli, now i got a clear picture of the pr but i have few confusions on |
Summary
This PR adds new [[build.env]] table to the builder.toml file. If a platform environment variable with the given key/name exists, it will be overridden at build time. Otherwise, a new build-time environment variable with the given name will be created. that will be translated to files in
/cnb/build-config/envby pack during pack builder create.end-users can change the
/cnb/build-config/envdirectory by definingCNB_BUILD_CONFIG_DIRenv variable when building imageOutput
when below table is added to builder.toml file
Before
After
Documentation
Related
Resolves #1632