Pass filename through to ERB and ignore .erb template extensions

[DavidS]

See https://github.com/ruby/ruby/blob/524fb0138b773f2ed01441abbcffeda0271175c5/lib/erb.rb#L916 for details.

This causes stack traces from ERB failures to point to the correct template.

[domcleal]

Thanks, it looks good, but would you mind adding a test to features/update.feature for a92b0c0 in particular?

There's already a test for similar .erb extension functionality circa line 715, so it should be easy to extend.

[DavidS]

Annoyingly https://github.com/voxpupuli/modulesync/blob/master/features/update.feature#L680 already has that test scenario. I'll need to dig into how that happened.

[domcleal]

I don't think that scenario (or the one above) is actually setting or using anything within the settings file, so it's only checking that the correct file on disk is modified without ensuring interpolation works as expected.

[DavidS]

bah - need more ☕ ; you're completely right of course.

[DavidS]

Updated the tests to actually pass through configuration info to the template/rendered file.

[domcleal]

All of the tests you changed seem to pass on master. Are you sure f2447c8 is fixing anything, or is it testing something different?

[DavidS]

What a rabbit hole - the bug I'm hunting only manifests itself when using a :global config block, as https://github.com/puppetlabs/modulesync_configs/blob/master/config_defaults.yml#L2 does. My fix addresses that issue, but the tests do not demo the fix.

[DavidS]

Meanwhile I've found another bug in that area, where https://github.com/voxpupuli/modulesync/blob/master/lib/modulesync/settings.rb#L36-L47 do not handle the .erb suffix properly.

I'm almost tempted to force-migrate everyone to *.erb files in moduleroot, and only support non-suffixed keys in the config.

[hunner]

It would be a breaking change, but if it would simplify the codebase and the time needed to maintain this in the future AND eliminate bugs at the same time, I'm okay with a one-time change of mv'ing everything to .erb

[james-powis]

Providing context, up until #93 .erb endings were not even used... basically every file was assumed to be ERB and rendered as such (even if there was no logic within them), #93 addressed a corner case problem and allowed for optionally name ending with .erb to prevent linters from accidentally picking up on directory level config files which contained ERB syntax causing failures.

When I submitted that PR I opted for complexity to prevent breaking changes... The Vox ModuleSync is widely used (even outside of vox), so I would caution about blowback of breaking the single way the module has worked until January of this year...

[DavidS]

I've uploaded #114 with a cleaned up version, and docs to implement the change as discussed here.

[domcleal]

Closing in favour of #114, which prefers .erb file extensions and deprecates support for files without the extension with a warning.

I do agree with @james-powis about the impact, especially as the change in #93 hasn't yet been released, however the implementation in #114 is rather clean, fixing a couple of bugs (such as this one) in the process. The non-extension behaviour is deprecated cleanly, so users can still use it without changes until the next major release.

[cdenneen]

So with this change all files must end with .erb in the moduleroot directory. But what happens in the case where you want an erb file to stay an erb file? Would this break it.
Guess curious as to why all files couldn't stay as is and anything you needed to be a template would require the .erb extension. (Still wouldn't help if you literally needed an .erb sync'd over)
So for example let's say we wanted to use modulesync for syncing many changes like these across control repositories. Where you'd like to default moduleroot/site/profile/templates/motd.erb to literally be an .erb on the destination side.

[DavidS]

@cdenneen as you correctly recognized, as long as .erb is recognized as marker for msync to evaluate templating (or, worse, everything is rendered) you won't have much luck with deploying ERB templates.

A nasty Workaround would be creating a template that renders a template, and call it something.erb.erb. This can be achieved by replacing all <% by <%= '<%' -%>. You could even wrap that up in a pair of rake tasks to encode/decode it for editing. I'll take no liability for blood and tears this causes.

Another evasion of this might be to use EPP templates instead. modulesync will not touch those.

A long-term solution on the modulesync side could be to have some kind of escape mechanism to tag *.erb files that need to be deployed without rendering.

[cdenneen]

@DavidS thanks... guess I'm still confused why the .erb extension for everything is necessary if it isn't a template that needs rendering. One would think this would make sync'ing quicker for something that doesn't require any sort evaluation.

[DavidS]

@cdenneen you don't need to use the suffix for files that do not need to be ERB rendered. They key breaking change was that filenames and key in the default_configs.yml need to match.

[cdenneen]

I had no keys in config and all files with .erb extension and wouldn't sync without it.

…

On Wed, Jun 7, 2017 at 5:05 AM David Schmitt ***@***.***> wrote: @cdenneen <https://github.com/cdenneen> you don't *need* to use the suffix for files that do not need to be ERB rendered. They key breaking change was that filenames and key in the default_configs.yml need to match. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#113 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAr84d8Ghl_-EBB27xX8Yuc1PL0Vx7Q4ks5sBmfVgaJpZM4NBh3C> .

[DavidS]

That's ominous. Can you write up a new issue for what exactly you've been seeing, and ping me on it?

…

On 7 June 2017 at 12:18, Chris Denneen ***@***.***> wrote: I had no keys in config and all files with .erb extension and wouldn't sync without it. On Wed, Jun 7, 2017 at 5:05 AM David Schmitt ***@***.***> wrote: > @cdenneen <https://github.com/cdenneen> you don't *need* to use the > suffix for files that do not need to be ERB rendered. They key breaking > change was that filenames and key in the default_configs.yml need to match. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#113 (comment) >, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAr84d8Ghl_- EBB27xX8Yuc1PL0Vx7Q4ks5sBmfVgaJpZM4NBh3C> > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#113 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABsQKrNWh_dq3dAOWsn-6lvJClckhaXks5sBocQgaJpZM4NBh3C> .