fix: set env and run scripts when starting cached image#359
Conversation
ab0ffa8 to
ed1ad13
Compare
ed1ad13 to
0e8b049
Compare
mafredri
left a comment
There was a problem hiding this comment.
I'll see about adding some tests tomorrow (need to figure out what to test), but in the meantime I think this is ready for review.
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?
| 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.
johnstcn
left a comment
There was a problem hiding this comment.
I tested the provider against this branch and all tests passed 👍
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.
aaa1965 to
b2041cc
Compare
(cherry picked from commit c11faf3)
Fixes #358