On Fri, Oct 10, 2014 at 10:02 AM, Daniele Sluijters <
[email protected]> wrote:
> Hi,
>
> Presumably you want the key => undef to represent that you want some
> sort of default value that is different from key not being set at all?
> There is yet another symbolic value that be used for that, the literal
> default. You can do Hash[String, Variant[Default, Integer]], and users
> can pass { a => default }, or {a => 42}, and if you specify
> Optional[Variant[Default, Integer]], the key can also be omitted.
>
> So yes, I want it to be initialised to 'nothing' essentially. The reason i
> tried undef was because I wanted to pass it on to an option on a type, exec.
>
> From my understanding of what you've written it would mean that this 3.x
> thing:
>
> $retries = undef
> exec { 'ls':
> tries => $retries,
> }
>
> Is the same as this in 4.x:
>
> $retries = default
>
> exec { 'ls':
> tries => $retries,
> }
>
> So essentially all the cases where we used to use 'undef' to specify a
> form of 'nothingness' so that the default for that parameter on the type we
> were passing it is was respected are now handled by undef.
>
>
I may be missing something, but I don't anything has changed. undef in 4.x
for resource parameters will be the same as in 3.x
I think Henrik was just saying that default is a value that can be passed
around now. And so if you wanted to make a distinction in your logic
between "default" and "not defined" you can.
> --
> Daniele Sluijters
>
> On Friday, 10 October 2014 07:28:09 UTC-7, henrik lindberg wrote:
>>
>> Thanks Daniele, excellent feedback !
>>
>> More comments below...
>>
>> On 2014-10-10 9:06, Daniele Sluijters wrote:
>> > Hi list,
>> >
>> > As an exercise to get me familiar with the new type system I started a
>> > from-scratch rewrite of the apt module (don't worry I won't publish it
>> > like that). The reasons for this are legion but it's mostly an exercise
>> > to get acquainted with the type system and rethink a few of the things
>> > we're doing, try out new stuff etc. All my testing has been done with
>> > Puppet 3.7.1's version of the future parser.
>> >
>> > First, some code (if the arrows don't line up it's a font thing, they
>> > line up in Vim just fine):
>> >
>> > class apt::params {
>> >
>> > $purge_defaults = { 'sources_file' => true,
>> > 'sources_dir' => true,
>> > 'preferences_file' => true,
>> > 'preferences_dir' => true, }
>> >
>> > $update_defaults = { 'policy' => 'changed',
>> > 'timeout' => undef,
>> > 'tries' => undef, }
>> >
>> > $proxy_defaults = { 'host' => undef,
>> > 'port' => 8080, }
>> > }
>> >
>> > class apt (
>> > Hash[Enum[sources_file,
>> > sources_dir,
>> > preferences_file,
>> > preferences_dir], Boolean] $purge = {},
>> >
>> > Struct[{policy => Optional[Enum[changed, always, daily, weekly]],
>> > timeout => Optional[Integer],
>> > retries => Optional[Integer],}] $update = {},
>> >
>> > Struct[{host => Optional[String],
>> > port => Optional[Integer[1,65535],}] $proxy = ${},
>> >
>> > Hash $sources = {},
>> > Hash $keys = {},
>> > ) inherits apt::params {
>> >
>> > $merged_purge = merge($purge_defaults, $purge)
>> > $merged_update = merge($update_defaults, $update)
>> > $merged_proxy = merge($proxy_defaults, $proxy)
>> >
>> > }
>> >
>> > As a rationale for all the hashes... What I want to have is not four
>> > purge_* parameters but one purge parameter with four keys. The user
>> > should then be allowed to supply a partial hash (meaning only the keys
>> > that need to change from the default) to configure behaviour (this is
>> > what the three merge() calls achieve in the body).
>> >
>> > First of all, this is awesome. Being able to express what kind of input
>> > you expect this way is great. Especially because you can go to great
>> > lengths to make sure that what you receive is what you would expect.
>> > Even though the Type code might be a bit much at this point it's going
>> > to spare us a whole load of headaches later on because we can just use
>> a
>> > variable/value and be sure that it's set to something sensible. Hurray!
>> >
>> > Unfortunately, if you look at the type code, even for the first Hash,
>> it
>> > quickly becomes difficult to read. It would be great if we could alias
>> > these things somehow, along the lines of:
>> >
>> > type purge_validation = Hash[Enum[sources_file, sources_dir,
>> > preferences_file, preferences_dir], Boolean]
>> > class apt {
>> > Purge_validation $purge = $::apt::params::purge_defaults,
>> > ) inherits apt::params { }
>> >
>> > I don't have a preference for a keyword but type seems sensible, alias
>> > could probably work too. Even though the validation code is still quite
>> > complex the class declaration itself becomes mighty easy to read, about
>> > as easy as the current form without all the type annotations.
>> >
>>
>> This is something we want to add in a 4.x version for the exact reasons
>> you mention - the long type specs in parameter declarations makes it
>> harder to read what is going on. To instead use a descriptive name is of
>> great value.
>>
>> > By the way, if you're wondering what that Type declaration says: I
>> > expect a Hash, that can contain 0-* keys (I did not specify a size on
>> > the hash so it is allowed to be empty). Those keys must be named one of
>> > these four things (those in the Enum[]) and I expect all values
>> > associated with those keys to be Boolean, so true or false.
>> >
>> > This means people can no longer torture you and your beautiful module
>> > with crap like this:
>> >
>> > class { 'apt':
>> > purge => { 'source_file' => 'yes', 'sources_dir' => 'false',
>> > 'preference_file' => 'UNDEF', 'preferences_dir' => true },
>> > }
>> >
>> > Puppet will simply throw errors at them. The errors themselves aren't
>> > very informative though. Currently you get them in the form of:
>> Expected
>> > parameter 'purge' to have type <the whole type definition here> but got
>> > <something else>. It would be nice if we could get error messages along
>> > the lines of: Expected parameter 'purge' to be <a more human
>> > description> but got <another more human description> I know that's a
>> > tall order, but on the list of "nice to have" I suppose.
>> >
>>
>> We have a ticket for that. The first, simple approach only provides
>> meaningful output for simple cases like "excpected String, got Integer",
>> but it breaks down for complex types leaving it up to the user to read a
>> large amount of type information to find the actual diff.
>> We want to do a much better diff output.
>>
>> > Onwards! Optional is causing me some trouble. According to the blog
>> > "Luckily, the type system has a type called Optional that does exactly
>> > what we want in this situation, it accepts something of a specific type
>> > or Undef."
>> >
>> > This would mean that notice(undef =~ Optional[Numeric]) should evaluate
>> > to true, and indeed it does:
>> > Notice: Scope(Class[main]): true
>> > Notice: Compiled catalog for nvc2542 in environment production in 0.33
>> > seconds
>> > Notice: Finished catalog run in 0.01 seconds
>> >
>> > So this should also work:
>> >
>> > class test (
>> > Optional[Numeric] $number = undef,
>> > ) {}
>> >
>> > include test
>> >
>> > However, it does not:
>> > Error: Expected parameter 'number' of 'Class[Test]' to have type
>> > Optional[Numeric], got Runtime[ruby, Symbol] on node nvc2542.
>> >
>> > I tried to change that to Variant[Numeric, Undef], even though that's
>> > exactly what Optional is defined as, but no dice either.. This feels
>> > like a bug to me, I'm hoping Henrik or Andy can shed some light on the
>> > situation.
>> >
>> Ah - you found a bug. Please file a ticket. We seem to have been a bit
>> too aggressive in removing the use of :undef.
>>
>> Until it has been fixed, you could try using a Runtime[ruby, Symbol] as
>> the type of undef in that particular situation.
>>
>> > One really awesome sauce feature of Optional is when it comes to
>> hashes.
>> > Earlier I showed this piece of code:
>> > Struct[{policy => Optional[Enum[changed, always, daily, weekly]],
>> > timeout => Optional[Integer],
>> > retries => Optional[Integer],}] $update = {},
>> >
>> > What I'm defining here is that I want a Hash (Struct[{}]), whose keys
>> > are named 'policy', 'timeout' and 'retries'. By setting their values to
>> > Optional however you are now allowed to pass in a partial hash, so just
>> > sending { 'policy' => 'changed' } into update will work without it
>> > complaining that you're missing the 'timeout' and 'retries' keys.
>> >
>> > One thing that does strike me as slightly odd though is that even
>> though
>> > the value is defined as Optional, which should allow us to send in
>> > undef, you cannot. If you do { 'policy' => undef } you'll get an error
>> > based on that validation. Now, I'm very glad it doesn't allow me to do
>> > so because that's really the behaviour I want in this specific case,
>> but
>> > that might not always be true.
>> >
>> > There are places where I really would like to allow undef as a value
>> for
>> > a hash key, but not always. I haven't found a way to express that yet
>> > though. So essentially want to be able to say both:
>> > - key may be omitted but if available must be of value Integer <-
>> > current behaviour of Optional[Integer] with a Structs{{ key => }]
>> > - key may be omitted but if available may be Integer or Undef <- I
>> > can't seem to express this. Though there is no need in the case of
>> > $update to pass in '{retries => undef}' a user should be allowed to do
>> > so even if it doesn't achieve anything and from my understanding of the
>> > Optional definition, it should.
>> >
>>
>> Presumably you want the key => undef to represent that you want some
>> sort of default value that is different from key not being set at all?
>> There is yet another symbolic value that be used for that, the literal
>> default. You can do Hash[String, Variant[Default, Integer]], and users
>> can pass { a => default }, or {a => 42}, and if you specify
>> Optional[Variant[Default, Integer]], the key can also be omitted.
>>
>>
>> > Lets add a bit to my confusion:
>> > class test (
>> > Struct[{policy => Optional[Enum[changed, always, daily, weekly]],
>> > timeout => Optional[Integer],
>> > retries => Optional[Integer],}] $update = {}
>> > ) {
>> >
>> > }
>> >
>> > include test
>> >
>> > 23:35:37 ~/D/g/d/p/apt (master) $ puppet apply test.pp --parser future
>> > Notice: Compiled catalog for nvc2542.nedap.local in environment
>> > production in 0.41 seconds
>> > Notice: Finished catalog run in 0.01 seconds
>> >
>> > It gets a bit weirder because this is valid too:
>> > class test (
>> > Struct[{policy => Optional[Enum[changed, always, daily, weekly]],
>> > timeout => Optional[Integer],
>> > retries => Optional[Integer],}] $update = { 'retries' =>
>> undef, }
>> > ) {
>> >
>> > }
>> >
>> > include test
>> >
>> > 23:35:58 ~/D/g/d/p/apt (master) $ puppet apply test.pp --parser future
>> > Notice: Compiled catalog for nvc2542.nedap.local in environment
>> > production in 0.42 seconds
>> > Notice: Finished catalog run in 0.01 seconds
>> >
>> >
>> > But:
>> > class test (
>> > Struct[{policy => Optional[Enum[changed, always, daily, weekly]],
>> > timeout => Optional[Integer],
>> > retries => Optional[Integer],}] $update = {}
>> > ) {
>> >
>> > }
>> >
>> > class { 'test':
>> > update => { 'retries' => undef, 'policy' => 'changed', 'timeout' =>
>> 1},
>> > }
>> >
>> > Results in:
>> > 23:38:01 ~/D/g/d/p/apt (master) $ puppet apply test.pp --parser future
>> > Error: Expected parameter 'update' of 'Class[Test]' to have type
>> > Struct[{'policy'=>Optional[Enum['changed', 'always', 'daily',
>> > 'weekly']], 'timeout'=>Optional[Integer],
>> > 'retries'=>Optional[Integer]}], got Hash[String, Runtime[ruby,
>> Symbol]]
>> > at /Users/daenney/Development/github/daenney/puppet/apt/test.pp:9 on
>> > node nvc2542.nedap.local
>> >
>>
>> Same bug as you found earlier. The evaluators notion of undef (which is
>> Ruby nil), is translated to the Compiler's notion of undef (Ruby symbol
>> :undef), but it is not translated back to nil when the type check is
>> made. (Same aggressive removal of :undef).
>>
>> And in case you wonder, the 3x function API has different rules :-)
>>
>> > At this point I'm utterly confused. It looks like I can't have Optional
>> > accept undef on a 'top' parameter, I can use it on a hash and
>> initialise
>> > that hash with a key that is set to undef but I cannot pass in a hash
>> > with that same key set to undef.
>> >
>> > Maybe I've misunderstood the behaviour of Optional or something is
>> going
>> > wrong in the way undef is being parsed, the Runtime[ruby, Symbol] I
>> find
>> > very suspicious, but someone should look at this and figure out what's
>> > going on. I'm betting the answer is going to be "you're being an idiot"
>> > but I would really like to understand why.
>> >
>> You are not an idiot. The problem is in the bridge between new code and
>> old code and the messy nature of undef in the old code.
>>
>> It is excellent that you found this, and I hope we can fix this in 3.7.2.
>>
>> > Except for my troubles with Optional all I have to say is
>> > "sweeeeeeeeet". As a module maintainer, this will prevent a lot of
>> > headaches. If only puppet-strings could parse a human-understandable
>> > description of the Type annotation into the docs it generates... :).
>> >
>>
>> Thanks Daniel for testing and sharing the experience. Much appreciated.
>>
>> - henrik
>>
>> --
>>
>> Visit my Blog "Puppet on the Edge"
>> http://puppet-on-the-edge.blogspot.se/
>>
>> --
> You received this message because you are subscribed to the Google Groups
> "Puppet Developers" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to [email protected].
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/puppet-dev/012970ad-d50b-497e-8c7c-fb9f7c9bfa97%40googlegroups.com
> <https://groups.google.com/d/msgid/puppet-dev/012970ad-d50b-497e-8c7c-fb9f7c9bfa97%40googlegroups.com?utm_medium=email&utm_source=footer>
> .
>
> For more options, visit https://groups.google.com/d/optout.
>
--
Andrew Parker
[email protected]
Freenode: zaphod42
Twitter: @aparker42
Software Developer
*Join us at **PuppetConf 2014, **September 20-24 in San Francisco - *
www.puppetconf.com
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/puppet-dev/CANhgQXte7U27r9CGXOrX_KWz51z-ud%2B6%2BtX-4XBnPYbOt1LgXw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
