fix: preserve files correctly when rendering to a local directory #268
Conversation
Signed-off-by: Kent Rancourt <[email protected]>
Signed-off-by: Kent Rancourt <[email protected]>
Signed-off-by: Kent Rancourt <[email protected]>
✅ Deploy Preview for docs-kargo-render-akuity-io ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #268 +/- ##
==========================================
+ Coverage 27.57% 27.72% +0.14%
==========================================
Files 22 22
Lines 2096 2103 +7
==========================================
+ Hits 578 583 +5
- Misses 1440 1441 +1
- Partials 78 79 +1 ☔ View full report in Codecov by Sentry. |
| if err = os.MkdirAll(outputDir, 0755); err != nil { | ||
| if err = copyBranchContents(rc.repo.WorkingDir(), outputDir); err != nil { |
There was a problem hiding this comment.
This is the main change, right here.
| func copyBranchContents(srcDir, dstDir string) error { | ||
| // nolint: gosec | ||
| if _, err := libExec.Exec( | ||
| exec.Command("cp", "-r", srcDir, dstDir), |
There was a problem hiding this comment.
cp may not exist in trimmed-down or distroless-like images, which is an eventual direction we need to consider for kargo or other utilities built on kargo-render. Do you think we could replace this with a golang implementation? e.g.:
https://stackoverflow.com/questions/51779243/copy-a-folder-in-go
or
https://github.com/otiai10/copy
or
There was a problem hiding this comment.
I'm open to that, but would note two things:
We use cp in other places besides this in both Kargo Render and Kargo proper (simply because the Go stdlib doesn't have a good alternative -- which would be why those libs all exist).
I love distroless and would much prefer that we we were distroless, but I believe the fact that we exec out to the gitCLI (for reasons I believe you're familiar with) present a much larger hurdle to achieving distroless than cp does.
Is this a bridge you'd be open to crossing at a later time?
There was a problem hiding this comment.
Yep, if we are already using cp else where, then there's no sense fixing it here and we can address it as separate feature.
When we write to a stage-specific branch, we gut its existing contents entirely and render into the empty directory. The long-time exception to that has been that we do not delete any files or directories explicitly named in the Kargo Render config as
preservedPaths.Only if writing to a local directory instead of a remote branch, all files that were meant to be preserved are going missing. The reason is that we have been rendering into a brand new, empty directory, when really, we should have been copying whatever was to be preserved from the target branch.
Note that since I worked on a new test case that involved a temporary directory, I have also taken the liberty of fixing a few outstanding temp directory-related nits.