-
Notifications
You must be signed in to change notification settings - Fork 32
feat: Add optional function to return manifest of generated files #225
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?
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.
If we're adding the ability to generate a manifest like this to the project, it should be an accessible option from the CLI as well. Regardless of how we tackle adjusting the backwards compatibility of the library, I think we should also look to expose that functionality in the boilerplate
CLI.
templates/template_processor.go
Outdated
// dependent boilerplate templates, and then execute this template. Note that we pass in rootOptions so that template | ||
// dependencies can inspect properties of the root template. | ||
func ProcessTemplate(options, rootOpts *options.BoilerplateOptions, thisDep variables.Dependency) error { | ||
func processTemplateInternal(options, rootOpts *options.BoilerplateOptions, thisDep variables.Dependency, returnManifest bool) (*manifest.Manifest, error) { |
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.
There's a more idiomatic way to do this, and I think it's worth breaking backwards compatibility to achieve it.
I'd recommend creating a NewTemplateProcessor
function that returns a template processor, accepting different options to configure it, then having a Process()
method to do what it needs to do.
This is an example of that:
https://github.com/gruntwork-io/terragrunt/blob/main/internal/discovery/discovery.go#L107
It also allows us to get out of passing around global options like that.
I'm not the lead maintainer of this project, so I'll solicit some feedback from other engineers at Gruntwork. I recommend waiting for their input before making any changes to this PR.
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.
We've discussed this internally, and I can bless the decision to engage in a breaking change to make this codepath more maintainable by changing the API.
We'll look to merge this in with another anticipated breaking change to require confirmation before running shells/hooks for users when in interactive mode.
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 has been updated. I just need some feedback as per below regarding the versioned manifest. Should be able to update early next week.
Also, I think using the CLI flag should write the manifest to disk, with some versioned schema that can be used by Boilerplate to do intelligent things like clean up files that were previously generated, but are no longer part of the template, etc. |
@yhakbar just to make sure I understand this correctly: When we talk about a versioned manifest file, we're essentially saying that on function invocation we're going to check if a manifest file already exists in the working directory, and if it does we update it, creating a versioned file in the process? Or do we expect the CLI flag to take in a directory input to where the manifest file needs to go/be read from? |
d6c854f
to
1887ae4
Compare
@yhakbar Added some changes to how I imagine the manifest could/should work with versioning. Open to feedback/suggestions. |
5c44c20
to
5797550
Compare
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.
Hey @tgeijg
I think there's some design work that we need to go through before we can get this merged.
The design decisions we need resolved are:
- What should the flag name be? What's the best name for it?
- Should the manifest remain a JSON file, and not a YAML file? Should that be configurable?
- Should the manifest be for a single generation, or multiple?
- Should the manifest have some way of indicating the schema that was used for the file?
Thanks for cutting this PR and your patience for the review!
you to run arbitrary scripts. | ||
1. **Cross-platform**: Boilerplate is easy to install (it's a standalone binary) and works on all major platforms (Mac, | ||
Linux, Windows). | ||
1. **Versioned file manifest**: Optionally generate a versioned JSON manifest that tracks all created files across multiple template generations. Each time boilerplate runs with `--output-manifest`, it adds a new version entry to the existing manifest file (or creates one if none exists), preserving the history of all previous generations for integration with other tools. |
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.
Why are we preserving all previous versions?
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.
I started there based on your previous comment: #225 (comment)
I think using the CLI flag should write the manifest to disk, with some versioned schema that can be used by Boilerplate to do intelligent things like clean up files that were previously generated, but are no longer part of the template, etc
We could add some arbitrary version limit for sure, not sure what we'd consider reasonable though. I use boilerplate exclusively for scaffold, for which this isn't really a required feature. I just need the manifest to address the issue of terragrunt formatting the entire working directory. I can't really comment on other use cases of scaffold and/or what would be reasonable limits to version history.
* `--no-shell`: If this flag is set, no `shell` helpers will execute. They will instead return the text "replace-me". | ||
* `--disable-dependency-prompt` (optional): Do not prompt for confirmation to include dependencies. Has the same effect as | ||
--non-interactive, without disabling variable prompts. Default: `false`. | ||
* `--output-manifest` (optional): Write a versioned JSON manifest of all generated files to `boilerplate-manifest.json` in the output directory. If a manifest file already exists, a new version entry will be added to track the current generation alongside previous ones. |
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.
Would a better name for this flag be --manifest
?
We don't require following these rules here, but if we can, let's go with --<system name>
when the name of the system can sufficiently capture what the flag is controlling. Other names could include --manifest-generate
or --manifest-file
if users can specify what they want the manifest file to be called, and specifying the name also determines that the file is generated.
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.
For clarity, are you suggesting we add multiple flags?
--manifest
returns the manifest, and
--manifest-file <file_name>
writes the file to <file_name>
?
The only problem I see with the latter is that we rely on a consistent file name for the history/version feature. If someone were to enter a file name with a typo you would lose your version history, or it would be incomplete the next time you run the command with the correct file name.
}, | ||
&cli.BoolFlag{ | ||
Name: options.OptOutputManifest, | ||
Usage: "Write a JSON list of all generated files to boilerplate-manifest.json in the current working directory.", |
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.
Did I request that this be in JSON instead of YAML? Seems at odds with the configuration file being in YAML, but it is a more convenient format in general. Maybe the format should be configurable. If we can get the UX to be similar to the Run Report that might be better, so there's an expectation of parity in how the two behave.
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.
That may have just been my default, this can be rewritten to yaml for sure. Will see how easy it is to make this configurable. Will have a look at Run Report.
forProductionExamplePath := "../examples/for-production/terragrunt-architecture-catalog" | ||
|
||
outputBasePath, err := ioutil.TempDir("", "boilerplate-for-production-output") | ||
outputBasePath, err := os.MkdirTemp("", "boilerplate-for-production-output") |
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.
We should replace this with t.TempDir
if we're going to replace this.
app := cli.CreateBoilerplateCli() | ||
|
||
outputPath, err := ioutil.TempDir("", "boilerplate-test-output-reqver") | ||
outputPath, err := os.MkdirTemp("", "boilerplate-test-output-reqver") |
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.
We should replace this with t.TempDir
if we're going to replace this.
require.NoError(t, err) | ||
|
||
// Wait a moment to ensure different timestamps | ||
time.Sleep(time.Second * 1) |
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.
Do we really need this?
t.Parallel() | ||
|
||
// Create temporary directory | ||
tempDir, err := os.MkdirTemp("", "manifest-test") |
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.
Use t.TempDir
t.Parallel() | ||
|
||
// Create a temporary directory for the test | ||
templateDir, err := os.MkdirTemp("", "template-test") |
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.
Use t.TempDir
t.Parallel() | ||
|
||
// Create a temporary directory for the test | ||
templateDir, err := os.MkdirTemp("", "template-test") |
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.
Use t.TempDir
defer os.RemoveAll(templateDir) | ||
|
||
// Create a temporary output directory | ||
outputDir, err := os.MkdirTemp("", "output-test") |
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.
Use t.Tempdir
Description
Fixes #224.
Adding a function to return the manifest of files that boilerplate generated. This would be the most clean way to work towards resolving this terragrunt issue.
I tried keeping things backwards compatible. The cleaner option would be to update theprocessTemplate
function to retun an output, but since that would change the function signature it would be a breaking change. Happy to make that change if desired.-- UPDATE --
As per comment from Yousif below, I introduced a breaking change instead of keeping things backwards compatible. Also expanded the manifest functionality to keep a versioned history, along with some possibly useful meta data.
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added function which returns a manifest of all generated files.
Migration Guide
Breaking Change: ProcessTemplate Function Signature
The
ProcessTemplate
function signature has been updated to return the list of generated file paths.Before:
After:
The function now returns a slice of file paths (relative to the output directory) for all files generated during template processing, followed by any error.
Required Action:
If your code directly calls ProcessTemplate, update it to handle the additional return value:
// Old code
// New code