-
Notifications
You must be signed in to change notification settings - Fork 49
New internal package for helper functions #588
New internal package for helper functions #588
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.
LGTM, but while we move things around, it would be great to add some tests, as it shouldn't be too hard.
96e7bc4
to
2fbb1a2
Compare
2fbb1a2
to
ad58fa2
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.
Just one nit, otherwise looks good, though the commits could be re-organized a bit. We could do:
- introduce new package
- switch existing code to use new package
- remove unused code
Adding a new internal package for utility and helper functions that need not be exposed outside. Currently there are lot of util packages in the codebase. Having an internal package helps in consolidating all such helper functions in this internal package rather than having separate util package/files in different places in the codebase. The first function that is to be added is RenderTemplate which renders provided go template. Adds unit tests for the same. Signed-off-by: Imran Pochi <imran@kinvolk.io>
Since RenderTemplate function is now part of the `internal` package, this caused duplication. Hence the file `template.go` is removed from the codebase. Also we update the references to `util.RenderTemplate` to now refer to the correct package and function. Signed-off-by: Imran Pochi <imran@kinvolk.io>
ad58fa2
to
f0f21f3
Compare
Commit 1: introduces new package and its tests |
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.
+1 LGTM
|
||
internaltemplate "github.com/kinvolk/lokomotive/internal/template" |
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.
Q: Why is it imported as internaltemplate
? Name conflicts?
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.
Can we import it as internaltemplate
everywhere else? So it becomes easier to read code?
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.
yes Suraj, name conflicts with Go library template
package.
This PR adds a new
internal
package that is not exposed to the outside world. The intention is to begin consolidating functions in variousutil
packages scattered in the codebase.The first function that is to be added is RenderTemplate which renders the provided go template.