On Wednesday, July 30, 2014 4:21:37 PM UTC-5, Andy Parker wrote: > > On Wed, Jul 30, 2014 at 1:02 PM, John Bollinger <john.bo...@stjude.org > <javascript:>> wrote: > >> On Tuesday, July 29, 2014 1:23:09 PM UTC-5, Andy Parker wrote: >> >>> On Thu, Jul 24, 2014 at 5:32 PM, Andy Parker <an...@puppetlabs.com> >>> wrote: >>> >>>> Howdy, >>>> >>>> Henrik, David, Erik, John, and others have been having some pretty epic >>>> conversations around resource expressions, precedence, order of >>>> evaluation, >>>> and several other topics. What kicked all of that off was us looking for >>>> some feedback on decisions we were making for the Puppet 4 language about >>>> how resource overrides, defaults, and so on actually work (or don't in >>>> some >>>> cases). I think we've finally reached some decisions! >>>> >>>> Henrik took all of the ideas and started trying to work out what we >>>> could do and what we couldn't. Those are in a writeup at >>>> https://docs.google.com/a/puppetlabs.com/document/d/ >>>> 1mlwyaEeZqCfbF2oI1F-95cochxfe9gubjfc_BXjkOjA/edit# >>>> >>>> Lots of information in there, so here is the summary. >>>> >>>> The principles behind the decisions: >>>> 1. Only make changes that have a high likelihood of *not* needing to >>>> be backed out later. >>>> 2. Strict or explicit is better than lax or implicit. It uncovers >>>> issues and keeps you from lying to yourself. >>>> 3. Puppet 3 has already stacked up a lot of changes. Do not break >>>> manifests unless we really have to. >>>> 4. Let's not break manifests and then have to break them in almost >>>> the same way once we start working on a new catalog system. >>>> >>>> There are three kinds of resource expression that we have to deal with: >>>> >>>> 1. Resource instantiation >>>> 2. Resource defaults >>>> 3. Resource overrides >>>> >>>> Looking forward, I think it is highly likely that the catalog system >>>> that we'll be working on during puppet 4 will be some sort of production >>>> (rules) system. In that kind of a world, resource instantiation likely >>>> remains as is, but defaults and overrides will end up having to change >>>> quite a bit, if not in syntax, at least in semantics. >>>> >>>> >>> Replying to myself...tsk tsk. Oh well. >>> >>> This is to provide an update on what's going on. Henrik put together a >>> bunch of changes to the implementation to carry out these decisions. I >>> worked on the specification to get it to align with these decisions >>> >>> The change to the specification happened in this PR https://github.com/ >>> puppetlabs/puppet-specifications/pull/14 >>> We've already identified one problem in the specification, it doesn't >>> allow nested arrays of strings in the title. I have a note to fix that. >>> >> >> >> Is support for nested string arrays an intentional current feature? I >> hadn't thought so, but maybe you slipped in automatic flattening of array >> titles sometime when I wasn't looking. If it's not a current feature, then >> does it need to be a future feature? >> >> > As a note: I've updated the tests to be much more explicit and cover more > of the functionality: > https://github.com/hlindberg/puppet/blob/PUP-2898_resource-expressions/spec/integration/parser/resource_expressions_spec.rb > > There are still some cases missing. Hailee (new developer at PL on the > team. She was an intern a couple times and worked on facter), is tackling > one bug and was going to check the tests against the specification and fill > in any gaps that I've missed. > > Allowing arrays of strings was definitely intentional ( > https://github.com/puppetlabs/puppet/blob/master/lib/puppet/parser/ast/resource.rb#L42). > > If the consequence of allowing nested arrays of strings was intentional is > harder to say. The tests for that code don't check for nested arrays and > the docs for resources don't mention nested arrays. However, it does work: > > > be puppet apply -e 'notify { [[a, b]]: }' > Notice: Compiled catalog for aparker.corp.puppetlabs.net in environment > production in 0.02 seconds > Notice: b > Notice: /Stage[main]/Main/Notify[b]/message: defined 'message' as 'b' > Notice: a > Notice: /Stage[main]/Main/Notify[a]/message: defined 'message' as 'a' > Notice: Finished catalog run in 0.06 seconds > >
Surprising. Leaving aside the tests for a moment, let's consider what the docs say: 1. They specifically say that resource titles are strings ( http://docs.puppetlabs.com/puppet/3/reference/lang_resources.html#syntax): "The title, *which is a string*"; "The title is *an identifying string*." 2. They say you can specify an array *of strings* as a resource title, as shorthand for multiple resource declarations ( http://docs.puppetlabs.com/puppet/3/reference/lang_resources.html#array-of-titles). This one is a little more open, but at best Puppet's behavior for titles consisting of arrays containing non-string elements (including other arrays) is undocumented. There are basically two approaches consistent with the docs. Puppet could either stringify non-strings presented as resource titles, or it could reject them. Current Puppet might do either one, depending on the specifics of the situation, and I agree that that inconsistency is a problem. I can accept the nested array behavior as the result of applying the array title rule recursively, though I don't think that's the most reasonable way to read the docs. If you are determined to keep it then the docs should be clarified. > The reasoning given by Henrik for why this should stay around is because > you couldn't previously directly concatenate two arrays in the puppet > language leading to people possibly doing this, either intentionally or > unintentionally: > > $a = [a, b] > $b = [c, d] > notify { [$a, $b]: } > > And of course this kind of a situation can be setup somewhere along a long > chain of constructing titles and passing things around, so it is really > hard to tell if it is really used or not. I did a check through a large > body of puppet code (a dump of the forge modules) and I haven't found > anything as explicit as what I have above, but every time I think something > isn't done it turns out that there is someone who was doing it. > > Just to note, that can now be done as: > > $a = [a, b] > $b = [c, d] > notify { $a + $b: } > > Any thoughts on how disruptive it might be to remove this? Or whether this > feature is even bad to keep around? It seems pretty harmless to me to just > keep it. > I think it's weird, and I think that although it has certain reasonable uses, it also provides for some surprising manifest development errors. I do not personally rely on the feature, but I cannot say to what extent, if any, others rely on it. I will not object to keeping it around, so long as you document it. > And that last one actually works? I would have expected this: >> >> ["[[nested, array]]" ,"resource", ["nestedarray"]] >> >> , which I don't categorize as "working". >> >> > Yep. That one creates two resources with those two titles. I'm willing to > change it and be stricter, but I think it comes down to making an argument > for why allowing nested arrays is actually *bad*. > I'm not prepared to present such an argument. > >> >>> #["{nested => hash}" ,"resource", ["{nested => hash}"]], doesn't >>> work for a literal hash (syntax error). In a variable it is created, but >>> isn't possible to reference the resource. deprecate? >>> #["[{nested => hash}]" ,"resource", ["{nested => hash}"]], works >>> as both literal and variable, but isn't possible to reference the resource. >>> deprecate? >>> >> >> >> You can't reference the literal because the resource name is not the >> hash, it is the stringification of the hash. IIRC works out to >> "nestedhash" in this case. I would be all for deprecating that behavior. >> As far as I know, it is never used except by mistake, and the errors >> resulting from that mistake are confusing to the casual Puppet user. >> >> > It works: > > > be puppet apply -e '$a = { my => hash } notify { $a: } notify { second: > require => Notify[{"my"=>"hash"}] }' > Notice: Compiled catalog for aparker.corp.puppetlabs.net in environment > production in 0.02 seconds > Notice: {"my"=>"hash"} > Notice: /Stage[main]/Main/Notify[{"my"=>"hash"}]/message: defined > 'message' as '{"my"=>"hash"}' > Notice: second > Notice: /Stage[main]/Main/Notify[second]/message: defined 'message' as > 'second' > Notice: Finished catalog run in 0.06 seconds > This is a bug. Per the docs, resource titles are strings. > > And it works in a surprising number of situations. The title of the > resource ends up being the hash: > It cannot (should not) be a hash. Resource titles are strings. > > { > "type": "Notify", > "title": { > "my": "hash" > }, > "tags": ["notify","class"], > "file": "/Users/andy/work/puppet/t.pp", > "line": 2, > "exported": false, > "parameters": { > "name": "{\"my\"=>\"hash\"}" > } > } > > The name parameter ends up being a stringification of the hash and edges > it is also a stringification: > > { > "source": "Class[main]", > "target": "Notify[{\"my\"=>\"hash\"}]" > } > > This can sometimes go wrong ( > https://tickets.puppetlabs.com/browse/PUP-2935 > <https://www.google.com/url?q=https%3A%2F%2Ftickets.puppetlabs.com%2Fbrowse%2FPUP-2935&sa=D&sntz=1&usg=AFQjCNFJCZBXkaSLbbeIsrMZO1cXlJi9MA> > > <- I haven't figured out what happened there other than using a hash as a > title going awry). > Perhaps that issue provides a clue, though, in that it describes different behavior under Ruby 1.8 than under Ruby 1.9. Does your test yield the same result under both versions? It may be that accepting a hash as a title without stringifying it results from a change in semantics between Ruby versions. Or since there was no test for the behavior, perhaps it was an unintended consequence of some other change. > Exactly. And that is why I'm a little nervous about just disallowing all > of these types. However, it will now fail fast and so the migration path to > the new system is: get rid of warnings, run with --parser future, fix > errors, check that your systems still work. OTOH what do we lose from just > continuing to stringify certain data types? > If you stringify only *certain* data types then you lose consistency, plus you fail to comply with the docs with respect to those that you do not stringify. You also give up an invariant on which otherwise you could rely. As I already said, I'm prepared to rationalize the behavior for nested arrays as a recursive application of the array-title feature. If you intend to keep that then I would prefer to see it documented that way, as opposed to as implicit array flattening. Considering it recursive involves less magic, even though both descriptions predict the same result. John -- 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 puppet-dev+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/26d753b3-3da4-459c-b021-9f7b9a0452cb%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
