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

Config templates #6

Merged
merged 28 commits into from
Mar 7, 2018
Merged

Config templates #6

merged 28 commits into from
Mar 7, 2018

Conversation

cam-stitt
Copy link
Member

@cam-stitt cam-stitt commented Mar 6, 2018

Fixes #5.

There were a few major changes in this release.

  • Migrated --vars option to --config, -c. This allows for future extensions to the configuration files.
  • Added template sections to configuration
  • Refactored the Render method to now utilize the visitor pattern from hil
  • Variables are now accessible at the var key and templates at the tmpl key.

Copy link

@callum-p callum-p left a comment

Choose a reason for hiding this comment

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

LGTM

config/config.go Outdated
type TemplateConfig struct {
Name string `hcl:",key"`
Content string `hcl:"content"`
Count int `hcl:"count"`
Copy link
Member Author

Choose a reason for hiding this comment

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

At the moment this isn't used. I should either decide on implementing the "count" template feature, or remove this field.

config/config.go Outdated
if err != nil {
return nil, err
}
err = interpolateVariables(vars, config)
Copy link
Member Author

Choose a reason for hiding this comment

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

Rename this to prepareVariables

"github.com/hashicorp/hil"
"github.com/hashicorp/hil/ast"
"github.com/openpixel/rise/config"
"github.com/openpixel/rise/interpolation"
)

// Template is a container for holding onto the ast Variables
type Template struct {
Copy link
Member Author

Choose a reason for hiding this comment

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

Now that we have an object of type "Template" part of config, it seems confusing to also have this object. I'm going to consider renaming this.

return n
}

if strings.HasPrefix(varName, "tmpl.") {
Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if I was to perform interpolation on each variable (by looking at prefix var. and then run the visit function on all new template nodes, if this would increase performance. At this point, I am visiting the tree just for templates and then running Eval on the tree, which visits the nodes again.

case *ast.VariableAccess:
varName = vn.Name
case *ast.Index:
if va, ok := vn.Target.(*ast.VariableAccess); ok {
Copy link
Member Author

Choose a reason for hiding this comment

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

I actually need to run the template function whenever a reference to *ast.VariableAccess is found. at the moment, if it finds a Target & Key, it will only run it once.

On top of that, when it finds an *ast.Index, that is when I need to look at adding a "index" variable to the substitution.

@cam-stitt cam-stitt merged commit d7f570e into master Mar 7, 2018
@cam-stitt cam-stitt deleted the config-templates branch March 7, 2018 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants