fix: set env and run scripts when starting cached image #359
Conversation
mafredri
force-pushed
the
mafredri/fix-cached-image-startup
branch
6 times, most recently
from
ab0ffa8 to
ed1ad13
Compare
mafredri
force-pushed
the
mafredri/fix-cached-image-startup
branch
from
ed1ad13 to
0e8b049
Compare
envbuilder.go
Outdated
| } | ||
| if buildParams.User != "" { | ||
| // BUG(mafredri): buildParams may set the user to remoteUser which | ||
| // is incorrect, we should only override if containerUser is set. |
There was a problem hiding this comment.
Review: Didn't want to change this logic now, but documenting it for posterity.
There was a problem hiding this comment.
Nice find. Can you open a follow-up PR to track this?
There was a problem hiding this comment.
I did and closed it right up: #365
I'll remove this comment.
There's still something wonky going on here:
envbuilder/devcontainer/devcontainer.go
Lines 301 to 305 in 4f3e9cd
But I don't understand its purpose yet. It seems to change the user at the end without actually doing anything with that user?
| os.Setenv(pair[0], pair[1]) | ||
| runtimeData.Scripts = devContainer.LifecycleScripts | ||
| } else { | ||
| opts.Logger(log.LevelError, "Failed to parse devcontainer.json: %s", err.Error()) | ||
| } |
There was a problem hiding this comment.
Review: We're parsing the workspace devcontainer.json here as we don't have access to the build-time devcontainer.json.
Motivation: Unless we cache the whole repo in the image, we can't reliably use devcontainer.json from it, it could be referencing unknown files from the repo which we haven't cached.
envbuilder.go
Outdated
| } | ||
| if buildParams.User != "" { | ||
| // BUG(mafredri): buildParams may set the user to remoteUser which | ||
| // is incorrect, we should only override if containerUser is set. |
There was a problem hiding this comment.
Nice find. Can you open a follow-up PR to track this?
|
|
||
| // Unset the Kaniko environment variable which we set it in the | ||
| // Dockerfile to ensure correct behavior during building. | ||
| _ = os.Unsetenv("KANIKO_DIR") |
There was a problem hiding this comment.
Review: This was left in a built image, but not in a cached image. I chose to go this path, but it also means it's not as "easy" to re-envbuilder inside a built image. But I didn't think that's really a use-case, wdyt @johnstcn?
There was a problem hiding this comment.
I think the main use-case of a cached image is to run the lifecycle scripts. We don't ever want to delete the filesystem or run kaniko inside a pre-built image anyway, so I don't see a reason to need this env.
There was a problem hiding this comment.
This isn't actually relevant for the cached image, since it doesn't set this env. It's only for a built image. But I'd argue the same logic applies there.
mafredri
force-pushed
the
mafredri/fix-cached-image-startup
branch
from
aaa1965 to
b2041cc
Compare
(cherry picked from commit c11faf3)
Fixes #358