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

ERB suffix handling #114

Merged
merged 3 commits into from
Apr 28, 2017
Merged

ERB suffix handling #114

merged 3 commits into from
Apr 28, 2017

Conversation

DavidS
Copy link
Contributor

@DavidS DavidS commented Apr 21, 2017

No description provided.

Fixing those complaints would require using an external library.
@DavidS
Copy link
Contributor Author

DavidS commented Apr 21, 2017

cc @james-powis @puppetlabs/modules @puppetlabs/sdk

@james-powis
Copy link
Member

All in all everything looks pretty concise and certainly a good cleanup of the code base. Per your description "prefer .erb in filename" certainly looks like it is required, none of the tests (or the code as I am interpreting it) allow for backward compatibility for non-.erb suffixed in module root. Was that the intention?

@DavidS
Copy link
Contributor Author

DavidS commented Apr 21, 2017 via email

@james-powis
Copy link
Member

I am really concerned about the level of work required by the community and members outside the community who leverage modulesync who are going to have to refactor because of this... it does not add much complexity to support non-.erb suffixed files, is there any way we can keep that logic but insist on the absence of .erb in the associated config files?

@DavidS
Copy link
Contributor Author

DavidS commented Apr 21, 2017

https://github.com/voxpupuli/modulesync/pull/114/files#diff-94f12d4c397c9e0adbe4eb57f1c8dcbeR13 still allows non-erb suffixed templates.

Fixing a template is as easy as find moduleroot/ -type f -exec git mv {} {}.erb \; && git commit -m "Update all files to .erb" I can add that to the CHANGELOG.

Copy link
Member

@rnelson0 rnelson0 left a comment

Choose a reason for hiding this comment

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

I approve the changes. However, I'd like some concurrence before merging, at least another set of eyes to ensure it is considered properly.

@bmjen bmjen requested a review from domcleal April 26, 2017 21:59
Copy link
Contributor

@domcleal domcleal left a comment

Choose a reason for hiding this comment

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

Can you add a test for the deprecated code path, where no ERB extension is given to the file in moduleroot/? Otherwise I think I'm OK with the change.

@DavidS DavidS mentioned this pull request Apr 28, 2017
#93 changed modulesync to allow for .erb suffixed templates. In many cases
this is useful to avoid the template being interpreted as the real thing
by other tooling, like e.g. a `.gitattributes` file.

As discovered around #113, the original fix was not complete, and e.g.
failed when being combined with `:global` sections. This commit now moves
more towards always using `.erb` suffixes on template's names, and - for
readabilitie's sake - only look for un-adorned keys in the config files.

Closes #113
@DavidS
Copy link
Contributor Author

DavidS commented Apr 28, 2017

@domcleal I've added the additional test, and I've created #115 as a separate PR (re-using the same commits) to make this PR here smaller, should this one need more work.

@domcleal domcleal merged commit 876cc80 into voxpupuli:master Apr 28, 2017
@domcleal
Copy link
Contributor

Merged, thanks @DavidS.

@DavidS DavidS deleted the erb-suffix-handling branch May 5, 2017 14:31
@bittner
Copy link
Contributor

bittner commented May 25, 2018

The README still says:

Defining templates

As commented, files within moduleroot directory can be flat files or ERB templates.

This is now incorrect, right? Also, the section title now seems weird. Maybe it should it better say:

Working with templates

As mentioned, files in the moduleroot directory are ERB templates (they must have an .erb extension, or they will be ignored).

PR?

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