-
Notifications
You must be signed in to change notification settings - Fork 80
Gomplate integration #629
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Gomplate integration #629
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces Gomplate integration to allow templating of local files. The implementation adds new functions and a command-line flag to control this feature. My review focuses on correctness and maintainability. I've identified a critical issue with unreachable code and a high-severity bug where the loaded Gomplate configuration is not being used. I've also included some medium-severity suggestions regarding the use of global variables and potential memory usage improvements for large files.
base/util/file.go
Outdated
| // if > v0.7 | ||
| return GomplateReadLocalFile(file) | ||
| // else | ||
| return io.ReadAll(file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block of code contains an unreachable return statement. The call to GomplateReadLocalFile on line 63 will always execute, making the io.ReadAll call on line 65 unreachable.
The comments // if > v0.7 and // else suggest that some conditional logic was intended here. This should be cleaned up to either implement the intended logic or remove the dead code and comments for clarity.
| // if > v0.7 | |
| return GomplateReadLocalFile(file) | |
| // else | |
| return io.ReadAll(file) | |
| return GomplateReadLocalFile(file) |
base/util/file.go
Outdated
| return err | ||
| } | ||
|
|
||
| gomplateRenderer = gomplate.NewRenderer(gomplate.RenderOptions{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The gomplateConfig is loaded from .gomplate.yaml in InitGomplateConfig but it's never actually used when initializing the gomplateRenderer. The gomplate.NewRenderer is called with empty RenderOptions. To apply the configuration from the file, you should pass it in the RenderOptions.
| gomplateRenderer = gomplate.NewRenderer(gomplate.RenderOptions{}) | |
| gomplateRenderer = gomplate.NewRenderer(gomplate.RenderOptions{Config: gomplateConfig}) |
base/util/file.go
Outdated
| var gomplateConfig *gomplate.Config | ||
| var gomplateRenderer gomplate.Renderer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using global variables like gomplateConfig and gomplateRenderer can make the code harder to reason about and test, as it introduces global state. While this might be acceptable for a command-line tool where initialization happens once at startup, consider refactoring this to avoid global state for better maintainability and testability. For example, you could have an initializer that returns a struct containing the renderer, which is then passed around where needed.
| } | ||
|
|
||
| dataIn, err := io.ReadAll(infile) | ||
| dataIn, err := baseutil.GomplateReadLocalFile(infile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GomplateReadLocalFile function is now being called on os.Stdin. This function reads the entire file into memory before processing. While this is consistent with how other files are handled, it could be problematic if a very large file is piped to stdin.
For streaming processing with gomplate, you could pass the io.Reader (infile) directly to gomplateRenderer.Render. This would avoid reading the entire input into memory at once.
// In GomplateReadLocalFile
// ...
err = gomplateRenderer.Render(context.Background(), file.Name(), file, &buf)5c848a6 to
fee2023
Compare
69cb34c to
c1fab1c
Compare
|
Just looking at the amount of lines changed here (all the vendoring), I don't think this is a realistic path forward. |
|
To make it easier to review/consider, please put all the vendored code in a single commit, the rest of the changes in another one. |
db0444a to
4a1bbd7
Compare
I believe you're talking about the whole concept of embeding gomplate, not the amount of commits I had 😓 ?
Done, the PR has now 3 commits
Thanks for your time and the review! Note: I didn't add the license on the newly created files for now, I'll add it once you're ok with what I did 😉 Edit:
|
4a1bbd7 to
606dace
Compare
|
Was able to add support for the I don't think Edit: Plugins also done, was easier than I thought |
- gomplate is opt-in via the `--enable-gomplate` flag - the only way to configure it is by providing a `.gomplate.yaml` file in the used `files-dir`
606dace to
9f9c493
Compare
Well, this PR was opened much earlier than intended due to my mistake; I accidentally opened it against the source repo instead of my fork 😓.
This PR proposes a solution to #111, as
envsubstmay not be sufficient for all users.This PR makes
butaneusegomplateas a templating engine, each filebutanewants to open is passed throughgomplatefirst.Gomplate is configurable by having a
.gomplate.yamlfile in the current directory, no flags are used to configuregomplate(for CLI simplicity).Disclaimer: This is my first time working with Go. I'm learning as I go, so things might not be perfect or fully optimized. Please be kind 🙏