feat: add template command (Ansible-style template rendering + copy) - #349
feat: add template command (Ansible-style template rendering + copy)#349robertobarreda wants to merge 12 commits into
Conversation
0d6079c to
041e7ac
Compare
umputun
left a comment
There was a problem hiding this comment.
neat idea, and it opens up some nice possibilities, per-host config with conditionals, generating init scripts, dropping a secret straight into a config file in one step. Worth saying up front that the simpler cases don't really need it, plain per-host content or a single value drop already works with a script heredoc (which does SPOT_*/env/secret substitution too), so template mainly earns its place on the Go-template logic and the one-step secret-into-config. Good direction overall, a few things to sort before merge, one of them a blocker.
1. not idempotent (blocker). with force: false the rendered file gets re-uploaded and overwritten on every run. copyPush skips on size+mtime+mode, but the temp file from os.CreateTemp gets a fresh mtime each run, so it never matches the remote and always re-uploads. For a config tool that churns mtime and can trigger service reloads/restarts. Compare rendered content against the remote (checksum), or set the temp file's mtime deterministically from the rendered bytes so an unchanged render actually skips.
2. single-quoted registered vars render the __SQ__: marker. maps.Copy(tplData, ec.cmd.Environment) at commands.go:657 copies env values raw, but a var set via a single-quoted export/setvar is stored as __SQ__:value, and only templater.apply strips that prefix (commands.go:890-900). So {{ .FOO }} for such a var renders the literal __SQ__:bar.
3. SPOT_ERROR is missing from the template vars. the SPOT_* map is hand-built at commands.go:643-656 and omits SPOT_ERROR, which templater.apply exposes. Both this and #2 disappear if Template() reuses templater's var-map + marker-strip logic instead of rebuilding it, so extracting that into a shared helper is probably the cleanest fix for both.
4. rendered files always land 0600, add a mode field. the temp file is 0600 and Upload chmods the remote to the source mode, so every rendered file is owner-only regardless of dst. A service running as another user can't read it. Add a mode: option, default 0600 so secret-bearing renders stay locked down and users set 0644 for plain configs. Don't preserve the .tmpl's own mode, a 0644 template would make a secret render world-readable.
5. missingkey=zero hides typos. an undefined key like {{ .APP_PROT }} renders empty with no error, so a config can silently ship listen ; or an empty secret. Switch to Option("missingkey=error") so a typo fails the command instead of writing a blank value.
smaller stuff:
resp.detailsshows the temp path as the source:{template: /tmp/spot-templateNNN -> /etc/...}. Rebuild it with the realsrcso the output means something.ecCopy.cmd.CopysetsSource/Dest/Direction, butcopyPushtakes the paths from its args and never readsDirection. Drop the three dead assignments.- docs: README uses
testdata/nginx.conf.tmplas the example src while llms.txt usestemplates/, picktemplates/in both (testdata/is a test-fixture convention). The "respectscondlike every other command" line isn't right (copy/sync/delete/wait don't support cond), just say it supportscond. Andvalidate's godoc list still omitstemplate. - tests: the register-vars-in-templates path is only covered by injecting
Environmentdirectly. Add one that drives aregisterscript through the runner into a template, with a single-quoted export so it catches #2. There's also no force=false/identical-content case.
tests and lint are green, and the secret handling checks out (rendered to a 0600 temp, cleaned up, never logged, no injection). No need to rework the copyPush reuse itself, the work is in the idempotence fix and the two var-map bugs.
3e01f32 to
bded9c1
Compare
Adds a new `template` command that renders a local Go text/template file with the command's environment, registered variables, loaded secrets, and the standard SPOT_* variables, then uploads the rendered result to a remote host. Reuses the existing copy-push path so mkdir, force, chmod+x and sudo work the same way as `copy`. Includes a new TemplateInternal config struct, a Template() method on execCmd, a dispatch case in Process.execCommand, schema updates, README and llms.txt documentation, and integration tests covering basic render, env, secrets, sudo, mkdir, chmod+x, error paths, and the cond field.
…m content hash Set the rendered temp file's mtime deterministically from SHA256(content) so that unchanged renders always produce the same (size, mtime, mode) tuple. Without this, os.CreateTemp's fresh mtime every run defeats the skip logic in sftpUpload and always re-uploads the file, which can trigger unnecessary service reloads/restarts.
Environment values with the __SQ__: prefix (set via single-quoted export/setvar)
were copied verbatim into the Go template data map, causing {{ .FOO }} to render
the literal prefix instead of the actual value. Replace maps.Copy with a loop
that strips the marker, matching templater.apply behavior for regular commands.
Replaces the duplicate SPOT_* + env var map construction in Template() and apply() with a single vars() method on templater. This fixes two bugs: - __SQ__: marker leaked into template rendering for single-quoted vars - SPOT_ERROR was not exposed to template data apply() still escapes $ for single-quoted env values by checking the original tm.env entry directly.
Rendered templates always landed with 0600 permissions because the temp file was created with os.CreateTemp (0600) and the upload path preserves the source mode. Add a mode: string field accepting octal permission strings like "0644", defaulting to "0600" so secret-bearing renders stay locked down while users can set 0644 for plain configs. The mode is applied via os.Chmod on the temp file before upload, so the existing upload path naturally picks it up.
Replace missingkey=zero with missingkey=error so that undefined template
keys like {{.UNDEFINED_VAR}} fail the command instead of silently
rendering empty strings. This prevents typos from shipping blank configs
or secrets.
Update related tests:
- template_basic.tmpl no longer references unused GREETING/MY_SECRET vars
- template_env.tmpl created for env/secret injection tests
- Add test verifying undefined keys produce execution errors
…d tests - Replace temp path with original src in template response details - Drop dead Source/Dest/Direction fields from synthetic CopyInternal - Inline single-use fileMode variable - Add template to validate() godoc list - Use templates/ consistently in README examples - Fix cond wording (not all commands support it) - Add test for single-quoted export propagation through register→template - Add test for force=false idempotency with identical renders
… Template - Remove unused cmdCopy and ecCopy variables in Script method - Use defer for temp file close in Template instead of explicit Close calls - Flatten SPOT_ERROR assignment in vars() to default-first pattern - Remove pointless nolint comment in scriptCommand
Build template command details directly from known values instead of rewriting copyPush's output string with fragile string replacements. Consolidate test path concatenation into single fmt.Sprintf calls.
Remove 73 lines of repeated boilerplate across 17 execCmd constructions by adding a makeEC helper and pre-computing testHost/testPort once. Only the ipv6 subtest keeps manual construction due to different host settings.
Replace ecSingle := ec (pointer copy) with ecSingle := *ec (value copy) in Mcopy, Msync, and Mdelete to prevent accidental mutation of the original execCmd.
Run make prep-site
bded9c1 to
d83ca52
Compare
Thanks for the detailed review. I tried to address all your concerns. |
umputun
left a comment
There was a problem hiding this comment.
most of the list is done. Items 2, 3 and 5 are fixed and the docs corrections are in. Four things still block, and two of them come out of the fixes themselves rather than the original feature.
1. still not idempotent (blocker, item 1). the content-derived mtime can't round-trip. commands.go:721 builds the time from 63 bits of the hash, so it's a Unix second anywhere up to ~9.2e18, but pkg/sftp sends mtime as a uint32: attrs := times{uint32(atime.Unix()), uint32(mtime.Unix())} in vendor/github.com/pkg/sftp/client.go:608. Only hashes that fall inside the uint32 range survive, which is a vanishing fraction of them. Common filesystems can't represent most of that range either.
two other paths still force an upload regardless of what the hash produces:
- sudo: the upload goes to
ec.uniqueTmp(...), a fresh random path each run, sosftpClient.Staton it misses and theisSameblock atremote.go:463never runs. The realdstis never compared at all, it just getsmv -fd over. - chmod+x:
isSamecompares mode, and the remote ends up with execute bits while the staging file stays 0600, so it can't match on any later run.
comparing the rendered content and the wanted destination mode against dst itself would cover all three.
2. the shared var-map changed substitution for every command (blocker). templater.vars() is the right idea, but apply now iterates it:
for k, v := range tm.vars() {map iteration order is unspecified and varies between runs. The old code applied the SPOT_* set in a fixed sequence and then env. Substitution is sequential text replacement, so the order decides the result whenever one value contains another variable's token. With env: {CFG: "{SPOT_TASK}.conf"} and dst: "/etc/{CFG}", repeated identical evaluations give both /etc/deploy.conf and the unexpanded /etc/{SPOT_TASK}.conf. Same playbook, different result, and apply is shared, so this reaches copy, sync, delete, script, wait, echo and line, not just template.
the merged map also lets an env key overwrite a SPOT_* built-in, which it couldn't do before.
keep vars() for the template data, but put back the explicit built-ins pass followed by the env pass in apply so existing behavior stays as it was.
3. mode is applied to the local staging file (blocker, item 4). I meant mode as the destination mode. The staging file needs to stay 0600. os.Chmod(tmpName, ...) at commands.go:714 can widen the temp file that already holds the rendered secrets, and sftpUpload then takes the remote permissions from that local stat (remote.go:513):
- on a runner with a shared temp dir,
mode: "0644"plusoptions.secretsmakes the rendered secret world-readable there for the length of the upload. - on Windows the default
0600doesn't hold either.os.Chmodonly toggles the readonly attribute, andos.Statreports any writable regular file as 0666 (os/types_windows.go), so 0666 is what gets applied on the remote. Windows is a released target in.goreleaser.yml.
these need to be two separate things: the staging mode, always 0600, and the wanted destination mode, compared and applied against dst on its own. Folding the second into the first is also what keeps item 1 broken, since a 0600 local file never matches a 0644 remote.
4. chmod+x is silently dropped when mode is set. chmodX := ec.cmd.Template.ChmodX && ec.cmd.Template.Mode == "" at commands.go:729. {mode: "0644", chmod+x: true} gives a non-executable file, reports success, and logs nothing. Config and validation accept both fields and the docs present them as independent options. Either fold the execute bits into the final mode or reject the combination with an error.
smaller stuff:
commands.go:693:defer tmp.Close()is registered before the remove defer, so LIFO runs the remove while the handle is still open. Windows opens files withoutFILE_SHARE_DELETE, so the remove fails and the rendered secret file stays in temp, with no retry after the close. Close before the upload, or use one deferred func that closes and then removes.- the nginx example can't run as written.
README.md:430sets noenv, buttemplates/nginx.conf.tmplatREADME.md:448uses{{ .APP_PORT }}and{{ .APP_ENV }}, and both are absent, so execution aborts on the first one. Same insite/docs-src/index.mdandsite/docs/llms.txt. Add theenv:block to the example or drop those keys from the template body. {{ range .List }}in that same block can't work either. Template data ismap[string]string, so.Listis either absent, which errors, or a string, and execution fails because text/template can't range a string.
on my earlier points that are still open:
- the dead
Source/Dest/Directionassignments are still atcommands.go:732.010c06a2's message says it dropped them but the commit doesn't touch those lines. - both tests I asked for are there, but neither pins what it's named for.
template force=false duplicates are skippedasserts the details prefix and the file content, and both hold whether the second upload was skipped or not, so it needs an assertion that tells a skip from a re-upload, which final content and mtime don't. The register test hand-injectsEnvironmentrather than driving aregisterscript and then a template throughProcess, so the dispatch arm inrunner.gois still not executed by any test and no fixture has atemplate:key. A playbook fixture running register then template would cover both.
the feature shape is good and I don't want it reworked. The variable and missingkey fixes are in good shape. The work left is the idempotence approach, putting apply's ordering back, and separating staging mode from destination mode, which also settles the chmod+x interaction.
Adds a
templatecommand that renders a local Go text/template file with env vars, registered variables, loaded secrets andSPOT_*variables, then uploads the result to a remote host. The upload reuses the existing copy-push path somkdir,force,chmod+x,modeandsudowork the same ascopy.New config struct
TemplateInternal{Source, Dest, Mkdir, Force, ChmodX, Mode}withexecCmd.Template()— full test coverage covering basic render, env, secrets, sudo, mkdir, chmod+x, mode, cond skip, missingkey error, idempotent re-upload, single-quoted export propagation, and error paths.Bugs fixed along the way: single-quoted
__SQ__:marker leaking into template data, missingSPOT_ERRORin template vars, undefined keys silently rendering empty (nowmissingkey=error), and pointer-vs-value copy bug inMcopy/Msync/Mdelete.Includes schema update (
templateSpecinplaybook.json), README and llms.txt docs.