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

Fixes #5378: We need a method for the mustache template #93

Conversation

peckpeck
Copy link
Member

@peckpeck peckpeck commented Aug 8, 2014

Copied from file_template_expand.cf

@@ -0,0 +1,49 @@
#####################################################################################
# Copyright 2013 Normation SAS
Copy link
Member

Choose a reason for hiding this comment

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

I think you mean 2014 :)

@jooooooon
Copy link
Member

Assuming you add the "mustache" attribute, and that there is an equivalent value for "non-mustache aka legacy" templates, I think this method has too much duplication with the original file_expand_template method.

I would prefer that we continue using the "absolutely DRY" principle we have been up to here, and introduce a new bundle called file_template_expand_type(tml_file, target_file, mode, owner, group, type), where the type argument specifies the "mustache" or "legacy" value. Then, both file_template_expand_mustache and file_template_expand could just be wrappers on this "core" method.

What do you think?

@jooooooon
Copy link
Member

Also, in this case, the file_from_template method should also be rewritten to wrap around the same "core" method.

@jooooooon
Copy link
Member

Last but not least, we should really have an automatic test (see the tests/acceptance) directory for this method too.

@peckpeck
Copy link
Member Author

peckpeck commented Aug 8, 2014

Updated

# @class_prefix file_template_expand_mustache
# @class_parameter target_file
#
# This bundle will define a class file_template_expand_mustache_${target_file}_{kept,repaired,error,ok,reached}
Copy link
Member

Choose a reason for hiding this comment

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

These last comments are not up to date with the classes that will be generated

@peckpeck
Copy link
Member Author

peckpeck commented Aug 8, 2014

updated

#
#####################################################################################

# @name File from template mustache
Copy link
Member

Choose a reason for hiding this comment

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

This looks kinda weird. This name will be displayed as the human name everywhere - in ncf-builder, and in Rudder's reporting. Can we reword it to "File from template (Mustache format)"?

@jooooooon
Copy link
Member

Aside from these 2 comments, this looks great! We're still missing a test though.

@peckpeck
Copy link
Member Author

peckpeck commented Aug 8, 2014

updated with test

jooooooon added a commit that referenced this pull request Aug 8, 2014
…e_mustache_template

Fixes #5378: We need a method for the mustache template
@jooooooon jooooooon merged commit 619c522 into Normation:master Aug 8, 2014
@peckpeck
Copy link
Member Author

PR rebased

@amousset
Copy link
Member

Wrong one!

@peckpeck
Copy link
Member Author

yes, just a failed test with rudder-dev

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