search

bartavelle / language-puppet

A library to work with Puppet manifests, test them and eventually replace everything ruby.
BSD 3-Clause "New" or "Revised" License
51 stars 8 forks source link

Strict Permissive mode and unknown/undef variable #103

Closed PierreR closed 9 years ago

PierreR commented 9 years ago

Puppet doesn't seem to complain about unknown variable and happily interprets them as undefined.

puppet apply -e 'notify {"hello $b":}'

Then there is this case:

puppet apply -e '$b = undef notify {"hello $b":}'

Would it be possible for puppetresources to bark in the first case in both permissive and strict mode (the variable is completely unknown) while the second case would be an error in strict mode only (the variable is known and its value is undef) ?

Hope this question is not too naive ...

bartavelle commented 9 years ago

It shouldn't be too hard, but it would mean that ALL variable resolution failures would stop interpretation, even in permissive mode. Is that OK ?

bartavelle commented 9 years ago

You just need to drop the ifStrict in the resolveVariable function in Puppet.Interpreter.Resolve.

PierreR commented 9 years ago

Ok I will play a bit with this setting and see how it goes.

Another option would be to be able to set a list of modules to be interpreted in permissive mode (but I guess this is harder to implement)

Here is my use case: I would love all my code to be interpret as strict but I don't mind (I actually need) the permissive mode for puppetlabs modules. I am only ignoring the really bad citizen (eg the maven puppetlabs module).

bartavelle commented 9 years ago

This kind of options would benefit from having a list of modules in a file (as this will get large), or the parser could be modified to detect some tag in the file comments ...

PierreR commented 9 years ago

You just need to drop the ifStrict in the resolveVariable function in Puppet.Interpreter.Resolve.

So far I love this better. The extra failures I have got had been ... well bugs. Do you think we need a third mode (permissive, lax, strict) for instance so we keep a mode that mimics puppet ?

This kind of options would benefit from having a list of modules in a file (as this will get large), or the parser could be modified to detect some tag in the file comments ...

Is this a lot of extra work ? I can currently cope with the first simple option for now (using a third mode or modifying the permissive one).

bartavelle commented 9 years ago

Which modification ? The comment one might be harder, and probably too useful, as you might not want to alter third party modules.

Having a new command line option that injects new preferences might not be horrible.

PierreR commented 9 years ago

Having a new command line option that injects new preferences might not be horrible.

Do you mean a general way to read option from a file instead of using the command line option ?

I guess this would be a nice new feature but I personally don't long for it that much because I already hide all these options inside shell script files (that calls a docker). So 'fix/static' options can already be read from a script file easily.

As far as I am concerned I don't mind a long list for now like we might have for the ignoremodules option.

I am a bit more concerned about the work it would require to make checkStrict read a list of modules. Is that easy ?

On the related but different subject what about three options "permissive, lax and strict" ? Does that sound like a good idea ? I am just wondering if I can go and drop the ifStrict in the resolveVariable as you suggested ? On one hand, this is better for testing in general on the other hand we won't have a mode that closely mimics puppet anymore. Hence the idea to add a third one if you still follow me ;-)

bartavelle commented 9 years ago

I personally don't care about being puppet-perfect. Having 3 modes is OK for me.

bartavelle commented 9 years ago

(Sorry for the terse answer !)

PierreR commented 9 years ago

I have done the most simple change for now keeping just 2 modes. I would add a third mode only if it proves to be useful later on.

I am going to open another issue for the second part of the discussion ("make checkStrict read a list of module").

I still have a small question ...

Is checkStrict equivalent to

ifStrict
  (throwPosError rr)
  (warn ("The variable" <+> pretty (UVariableReference fullvar) <+> "was not resolved" <+> pretty PUndef <+> "returned") >> return PUndef)
bartavelle commented 9 years ago

Yup : https://github.com/bartavelle/language-puppet/blob/b69c9b35ec27f497d2a730ced64aeb4ac875764a/Puppet/Interpreter/Types.hs#L851-L859