Repository navigation
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
Binary size report (
|
| Size | |
|---|---|
Base (main) |
33MiB |
| PR (#2327) | 33MiB |
| Delta | +14KiB (0.04%) |
|
Hey @vic1707, thanks for putting this together! This is much closer to what I had in mind. I like that it’s opt-in, keeps the templating dependencies out of Butane, and invokes the processor directly without a shell. The stdin/stdout approach and failing translation when the processor fails make sense to me too. A few things I think we should work through before settling the interface:
Once we’ve agreed on those pieces, it would be good to add tests for processor failures, missing executables, disabled processing, and local-file handling alongside the happy-path test. I ran the Butane tests and vet locally, and those passed. There’s also a small lint failure on the unchecked All that to say, I like this direction. I think the main thing now is agreeing on which files the processor sees and what context we give it. What do you think? |
|
Hi @prestist 👋, long time no see, hope you're doing fine. Pushed 3 changes on what doesn't need more discussion (imo), the lint fix, putting the reader in options rather than module-global, and add filename to errors (error examples available in the commit description). Remaining subjects would be 1. which files are processedToo many edgecases imo
Introducing an extension allowlist for processing (either as env or proper arg 2. processor tools may ask/want additional settings (that can't be expressed via env vars which are already inherited)I'm mainly thinking of note: current implementation makes 3. inter-file processor context?Concern that may only apply to me, and that was an issue on the last PR too. What if I want to define a variable at runtime (ie: a secret app random token) that I need in multiple imported files? currently this context is lost. I don't see a way to fix that other than remaking the source.bu with every file-based import being turned into an inline-based import. I would consider this out-of-scope and users would have to figure that out on their own (with 4. tools that don't accept
|
gives
```
failed to read poc/file-processor/root-error.bu: file processor "/Users/bofzilla/.local/share/mise/installs/gomplate/5.2.0/gomplate" failed: exit status 1
```
on error in source.bu
and
```
{"time":"2026-09-25T10:43:09.428835+02:00","level":"ERROR","msg":"","err":"renderTemplate: failed to render template -: template: -:1:3: executing \"-\" at <fail \"local render failure\">: error calling fail: template generation failed: local render failure"}
error[$.storage.files.0.contents.local]:
--> poc/file-processor/local-error.bu:7:16
|
5 | - path: /etc/message
6 | contents:
7 | local: local-error.txt
| ^^^^^^^^^^^^^^^ reading poc/file-processor/local-error.txt: file processor "/Users/bofzilla/.local/share/mise/installs/gomplate/5.2.0/gomplate" failed: exit status 1
8 |
|
Error translating config: source config is invalid
```
on local file read
97fc47b to
fd4b063
Compare
|
Hi, may I ask for an update on this @prestist / @angelcerveraroldan ? |
|
I think that it may be nice to keep the API really simple to start with, and add things later if we see the need for them. A good starting point IMO would be to assume that the pre-processor uses stdin-stdout, and not pass any args to it. If someone wanted to pass args to their tool, or if their tool does not use stdin-stdout, they can always make a simple wrapper scripts that handles that.
I think that this is the harder question. I think that an extension whitelist is nice option, as it avoids accidentally processing binary files / files that were meant to be processed by someone else. As for cases when the main butane file is passed through stdin, I would lean towards processing that too, but I'll have to think more about this, I am not too sure if I am missing any edge cases / weird UX. |
Hi,
following up on #2298 latest idea @angelcerveraroldan @prestist if you want to take a look.
Keeping it as draft for illustration purposes, assuming this idea is kept I'll close the Gomplate focused PR in favor of this one.
I'm starting to have a number of open PRs, sorry about that