Skip to content
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

Templating Update #358

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

Templating Update #358

wants to merge 23 commits into from

Conversation

dahendel
Copy link

@dahendel dahendel commented Jul 5, 2024

This PR adds go templating support while aiming to not introduce any breaking changes to the current gitops-templates repository or code functionality.

It also adds detokenize_test.go which clones a template repo and then executes the DetokenizeGitGitops function calls on the supplied path.

Introduces Tokens interface to help reduce duplicate code.

The code works as follows:

  • When a file is being processed a regex.ReplaceAllStringFunc is called to update the current template tokens and convert them to a go templatable format. ie: <GITOPS_REPO_URL> becomes ${K1 .GitopsRepoURL }
  • When all of the tokens are converted it is parsed with text.template, then finally Executes the template with the new parsedTemplate contents and passes the tokens.
  • Writes the File with the rendered contents

@DrummyFloyd
Copy link
Contributor

seems really cool like this , even if the delimiter [[ is bit weird to see ^^.

overall, you should try to clean a lil bit all the tabs/ new line , it's kinda hard to read clearly all the changes.
but i think the breaking change is quite huge, don't know how K1 teams will handle this , through gitops-template

@dahendel
Copy link
Author

dahendel commented Jul 7, 2024

@DrummyFloyd thanks for the feedback. This still works with all the current templating in place, to include the cert manager annotations. I am working on cleaning it up a bit more but wanted feedback before I went too far.

Copy link
Member

@patrickdappollonio patrickdappollonio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving some general comments. Thanks a lot for your help answering my questions on Slack, @dahendel!

Comment on lines 52 to 54
// If no match found, return the original input as a string, so we can have a visual indication
// that the token was not found in the Token Struct without erroring out
return "<variable-not-found>"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the comment is off, since the return does not return the original input as a string, but the hardcoded string instead (the original input could've been <FOO_BAR> but it won't return that).

Comment on lines 26 to 55
func ToTemplateVars(input string, instance Tokens) string {
value := reflect.ValueOf(instance)
if value.Kind() == reflect.Ptr {
value = value.Elem()
}
sanitizer := strings.NewReplacer("<", "", "_", "", ">", "", "-", "")

fields := value.Type()
normalizedName := strings.ToLower(sanitizer.Replace(input))
for i := 0; i < fields.NumField(); i++ {
field := fields.Field(i)
val := value.Field(i)

if normalizedName == strings.ToLower(field.Name) {
// If the field value is the zero value of the field template
// will add - to the template variable to clean up extra whitespace.
// We do not have any bool values in the tokens, so we should be ok.
// TODO: Find a better way to handle this.
if val.IsZero() {
return fmt.Sprintf("%s- .%s %s", leftDelimiter, field.Name, rightDelimiter)
}
// if field name matches return the correct formatted name
return fmt.Sprintf("%s .%s %s", leftDelimiter, field.Name, rightDelimiter)
}
}

// If no match found, return the original input as a string, so we can have a visual indication
// that the token was not found in the Token Struct without erroring out
return "<variable-not-found>"
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function would definitely benefit from a table test, especially for the zero-value handling to ensure other devs don't break the code.

return err
}
func replaceTemplateVariables(content string, tokens Tokens) string {
regex := regexp.MustCompile(TokenRegexPattern)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To prevent paying the compilation price, the MustCompile could be called outside of the function once and save some CPU cycles 😄

Comment on lines 142 to 143
funcs := sprig.GenericFuncMap()
funcs["toJson"] = toJson
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Masterminds/sprig includes a toJson function already and the function below seems generic enough

Comment on lines 67 to 68
func detokenize(path string, tokens Tokens, gitProtocol string, useCloudflareOriginIssuer bool) filepath.WalkFunc {
return filepath.WalkFunc(func(path string, fi os.FileInfo, err error) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to have been there from before but the path string from func detokenize() is never used, since the path string inside filepath.WalkFunc is not the same as the top one.

Comment on lines 27 to 28
leftDelimiter = "${k1"
rightDelimiter = "}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussing with the rest of the team, we want to go with these for delimiters:

Suggested change
leftDelimiter = "${k1"
rightDelimiter = "}"
leftDelimiter = "<%"
rightDelimiter = "%>"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants