fix: expand cache probe support to more directives #17
Conversation
mafredri
force-pushed
the
mafredri/fix-expand-cache-probe-support
branch
2 times, most recently
from
19cd569
to
e20fba2
Compare
mafredri
force-pushed
the
mafredri/fix-expand-cache-probe-support
branch
from
e20fba2
to
63195d1
Compare
| } | ||
| logrus.Infof("Changed working directory to %s", config.WorkingDir) | ||
|
|
||
| // TODO(mafredri): Check if we need to do more here. |
There was a problem hiding this comment.
If this command runs in a build (uncached) and the target directory doesn't exist, it will be created and added to w.snapshotFiles. My concern is if we need to mimic this behavior or not when running cached.
Testing or experience should tell us, though, so maybe I'll just remove the TODO.
There was a problem hiding this comment.
I'm fine with leaving TODO here, we can always remove it later. Thanks for sharing the context!
| @@ -79,6 +79,30 @@ func (w *WorkdirCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile | |||
| return nil | |||
| } | |||
|
|
|||
| func (w *WorkdirCommand) CachedExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error { | |||
| logrus.Info("Cmd: workdir") | |||
There was a problem hiding this comment.
This is copy paste from the uncached execute, felt it's better to mimic the behavior.
| existingVolumes[volume] = x | ||
| util.AddVolumePathToIgnoreList(volume) | ||
| } | ||
| config.Volumes = existingVolumes |
There was a problem hiding this comment.
maybe add logrus.Info() to indicate which volumes will be ignored while building?
There was a problem hiding this comment.
This logic is copy paste from the uncached execute command. I don't think we should alter the behavior here 🤔
There was a problem hiding this comment.
I was thinking only about improvements of debugging life, but it can be left as is 👍
There was a problem hiding this comment.
That's valid. We could always add it to both but maybe that's suited for a separate debugging PR.
Fixes coder/envbuilder#254