modify loader to add extra fields after loading #16082
Conversation
|
@modular-magician assign-review @melinath |
|
Hello! I am a robot. Tests will require approval from a repository maintainer to run. Googlers: For automatic test runs see go/terraform-auto-test-runs. @melinath, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR hasn't generated any diffs, but I'll let you know if a future commit does. |
| } | ||
|
|
||
| loader := loader.NewLoader(loader.Config{Version: version, BaseDirectory: baseDirectory, OverrideDirectory: overrideDirectory, Sysfs: ofs}) | ||
| loadedProducts := loader.LoadProducts() |
There was a problem hiding this comment.
I may be missing the "why" of the change overall, but I had a slight preference for the well encapsulated, low coupling previous version of the code.
loadedProducts := loader.LoadProducts() lets the loader do everything behind the scenes and return the products.
Could you keep that? It seems a bit "complicated" for users of the Loader type to need to know that they need to call LoadProducts, AddExtraFields, and Validate in that order, then access .Products.
I think I preferred having all of those methods private implementation details, and only have one public LoadProducts().
The rest of the change LGTM.
There was a problem hiding this comment.
@nicdumz we need to be able to load the yaml files, modify them in-memory, and write them out again - which means we need to separate loading from any "post-processing" like AddExtraFields.
| loader := loader.NewLoader(loader.Config{Version: version, BaseDirectory: baseDirectory, OverrideDirectory: overrideDirectory, Sysfs: ofs}) | ||
| loadedProducts := loader.LoadProducts() | ||
| loader.LoadProducts() | ||
| loader.AddExtraFields() |
There was a problem hiding this comment.
@nicdumz pointed out this returns an error that should be handled.
4 participants
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.