refactor(fqdn): encapsulate FQDN template logic into TemplateEngine - #6292
Conversation
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Pull Request Test Coverage Report for Build 23435131007Details
💛 - Coveralls |
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
| annotationFilter string | ||
| fqdnTemplate *template.Template | ||
| combineFQDNAnnotation bool | ||
| templates fqdn.TemplateEngine |
There was a problem hiding this comment.
Using templates can make you think it's an array of fqdn.Template.
Since it's a fqdn.TemplateEngine, shouldn't the variable be named templateEngine ?
There was a problem hiding this comment.
Thanks for flagging. Changed everywhere.
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
|
I renamed fqdn package to templateenginge |
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
| limitations under the License. | ||
| */ | ||
|
|
||
| package templateegine |
There was a problem hiding this comment.
| package templateegine | |
| package template |
There is a typo in the directory name & package name.
It should be at least templateengine.
Since the type is now named Engine, the package could be named template so it would be called in all sources with template.Engine.
Wdyt ?
There was a problem hiding this comment.
Yeah. Make sense
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
|
moved template engine test utils under source/template/testutil/testutil.go. Source tests no need to be aware of template engine creation cbf39cf |
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
mloiseleur
left a comment
There was a problem hiding this comment.
Looks very clear now 👍.
/lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ivankatliarchuk The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What does it do ?
Follow-up
Motivation
One place to change. Adding a fourth template field, changing combine semantics, adding a new template function - all changes land in fqdn/ and propagate automatically to all 10+ sources.
FQDN template handling was duplicated inline across ~10 source files — each called a free function with inconsistent guarding and accessed the engine's internals directly. There was no single place to reason about template configuration or behavior.
More