Fix multiline wait swallowing errors, wire delete exclude through#352
Conversation
umputun
left a comment
There was a problem hiding this comment.
verified both fixes independently: the new wait test indeed fails on master, and the exclusion rewrite checks out except for the items below.
- the glob branch in
isExcludedexcludes plain files matching the dir-glob prefix:da*/*now excludes a file nameddata.txt(checked old vs new: false -> true). The helper is shared with copy/sync/upload, so their exclusion behavior silently changes too. Both walkers haveIsDirat hand, pls thread it through sodir*/*only protects directories - zero-match exclude list still deletes the whole tree silently (pkg/executor/remote.go:267-270, matches local). A typo'd keep-pattern deserves a
[WARN] no exclusion pattern matched anything under ...before the tree goes away - exclude on a non-recursive delete is silently ignored without sudo but hard-errors with sudo. Worth making it symmetric, reject exclude without recur the same way
normalizeSlasheskills backslash escapes in patterns (foo\*barno longer matches a literalfoo*bar). Fine as a contract for a deployment tool, just add a README line that backslashes are treated as path separators and escapes are unsupported- the sudo+exclude error doesn't name the offending location, worth including it for mdelete with several entries
nits: isExcluded deserves the same godoc its sibling got, exclude patterns get re-normalized on every segment iteration (normalize once before the loop), the inner path.Match shadow can be flattened with trimmed, found := strings.CutSuffix(ex, "/*"), and README gained "honour" where the rest uses American spelling.
btw the runtime placement of the sudo+exclude guard is right, task-level sudo propagates after config validate, so no change needed there.
66a93bc to
4f78b2a
Compare
|
Addressed in 4f78b2a — all five points plus the nits.
Nits: The empty-ancestor-dirs case for a partially-matching mixed exclude set is documented in the PR body as a known cosmetic limitation (no data lost or wrongly deleted). CI green. |
umputun
left a comment
There was a problem hiding this comment.
confirmed all five round-1 items and the nits are addressed, verified the isDir fix empirically (da*/* no longer catches data.txt, dir exclusion intact), full suite green here including container tests. The gating introduced two new issues though:
- the remote zero-match
[WARN]fires on every recursive remote delete, exclude or not (pkg/executor/remote.go:269-273). Local is protected by thelen(excl) == 0early return, remote has no such guard, and that path includes the temp-dir teardown after every multiline script/wait, so every deployment log gets the warning. Pls gate it on a non-empty exclude list, and a test asserting no warn for exclude-free deletes would pin it - remote
Upload(remote.go:80) andfindMatchedFiles(remote.go:670) hardcodeisDir=false, but glob matches can be directories.exclude: ["subdir/*"]used to skip dirsubdiron master and still does inLocal.Upload(it stats each match), while remote now feeds the dir into sftpUpload and fails. Pls stat the matches and pass the real value, os.Stat for upload (globs are local paths) and sftp Stat for download
also README changed but site/docs-src/index.md wasn't regenerated, needs make prep-site (it still carries the round-1 wording incl. "honour").
minor, non-blocking: Local.Upload now stats before the exclusion check, so an excluded broken symlink fails the whole upload instead of being skipped. On stat error worth trying the match with isDir=false and skipping if excluded.
Previously, a multiline wait command that timed out was reported as successful when the remote temp-dir cleanup succeeded, because the teardown defer overwrote the returned error with nil. Previously, the delete command ignored its documented exclude patterns and removed files the playbook asked to keep. Wiring it through: - isExcludedSubPath now protects any ancestor of an excluded path, not just the immediate parent, so exclusions nested deeper than one level survive. - Remote recursive delete removes the whole tree when no exclude pattern matched, matching the local executor, and logs a WARN in that case so a mistyped pattern is not silently destructive. - The dir-glob rule (a 'dir/*' pattern also protecting the directory it names) is now gated on the entry being a directory, threaded through all isExcluded call sites, so a pattern like 'dir*/*' no longer excludes a plain file such as 'data.txt'. - Exclusion matching uses forward-slash semantics on all platforms, so documented patterns work with windows local paths too. - exclude requires recur and is rejected without it, symmetric between sudo and non-sudo; sudo+exclude is still rejected. Both errors name the offending location, and mdelete validates all entries before deleting any of them. The wait teardown fix mirrors Script: the teardown error is only surfaced when there is no primary error.
- Gate the remote no-match delete WARN on a non-empty exclude list, so an exclude-free recursive delete (e.g. temp-dir cleanup after every multiline script or wait) no longer logs a spurious warning; local was already guarded by its len(excl)==0 early return. - Remote Upload and findMatchedFiles now stat each glob match and pass the real isDir to isExcluded, so a directory exclude like 'subdir/*' skips a matched directory instead of feeding it into sftpUpload/Download and failing. Local.Upload already stats; it now also checks exclusion before reporting a stat error so an excluded broken symlink is skipped rather than failing the whole upload. - Regenerate site/docs-src/index.md from README (make prep-site). Tests: no-warn for exclude-free recursive delete, warn plus full removal for a non-matching exclude, and a glob upload that excludes a matched directory (fails without the stat).
4f78b2a to
1e9cc15
Compare
|
Round-2 addressed in 1e9cc15 (rebased on current master).
Full executor + runner suites green here including container tests; the dir-exclude upload fix negative-checked (fails without the stat). CI green. |
umputun
left a comment
There was a problem hiding this comment.
both blocking items are sorted, verified: the remote warn is gated on len(exclude) > 0 with the whole-tree fallback still firing for exclude-free deletes, and Upload/Download now stat each match (os.Stat local, sftp Stat remote) so dir-glob excludes skip a matched directory. The new remote_test.go cases pin all of it. LGTM.
two minor leftovers, non-blocking, up to you:
- the
filepath.Relerror paths wraperr(nil there) instead of the locale, so a rel failure would print%!w(<nil>)and lose the cause. Three spots: local.go:99, remote.go:78, remote.go:677. Pre-existing, adjacent to what you touched - the download-side dir-glob fix (findMatchedFiles) has no test of its own. The Upload one covers the symmetric path, but a mirror test excluding a matched directory on download would pin it directly
happy to see these in a follow-up or here, whatever you prefer.
Multiline
waitswallowed its errorPreviously, a multiline
waitcommand that timed out was reported as successful whenever the remote temp-dir cleanup succeeded: the teardown defer assigned its result to the named return unconditionally, overwriting the timeout error with nil (single-line waits were unaffected as they have no teardown). The defer now only surfaces the teardown error when there is no primary error, mirroringScript. A newwait multiline failedtest fails on the old code and passes now.deleteignored documentedexcludepatternsPreviously, the runner dropped the
excludefield ofdelete/mdeletecommands and passed onlyrecurto the executor, so files the playbook asked to keep were deleted. Wiring it through surfaced several latent executor issues, all fixed here:isExcludedSubPathonly protected the immediate parent directory of an excluded path, so exclusions nested deeper than one level were removed together with their ancestor directory; it now protects any ancestor of an excluded path.dir*/*matched the files inside but not the directory itself, so local delete removed the whole directory before visiting the excluded children; the directory rule is now glob-aware./-separated patterns also work with Windows local paths.The
sudo+excludecombination is rejected with an explicit error (formdeletebefore any location is deleted), since sudo deletion runs a plainrmthat cannot apply exclusion patterns; README notes this limitation.Known cosmetic limitation: when an exclusion pattern of a mixed set matches nothing (or a wildcard pattern like
*/keep.logmatches only some directories), empty ancestor directories can be left behind. No data is lost or wrongly deleted in these cases.Split out of #350.