On Wed, Jul 30, 2014 at 1:02 PM, John Bollinger <john.bollin...@stjude.org> 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 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. > >> >> The implementation changes are currently in this PR https://github.com/ >> puppetlabs/puppet/pull/2914 >> I'm working through adding tests based on the specification and to >> explore what kinds of changes to the current semantics we have got. >> >> Joshua and I have been working on putting together some tests to not only >> test the implementation against the specification, but also to see how >> different it really ends up being from the old implementation. The tests >> check several different scenarios, but basically, the first item is used as >> the title of a resource, the second is whether we expect to create a >> resource or get an error. The third element is either the regexp for the >> error message or the titles of the resources that actually get created. >> >> This is for the current system: >> >> ["thing" ,"resource", ["thing"]], >> ["[thing]" ,"resource", ["thing"]], >> ["[[nested, array]]" ,"resource", ["nested", "array"]], >> > > > 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*. > > >> >> ["1" ,"resource", ["1"]], # deprecate? >> ["3.0" ,"resource", ["3.0"]], # deprecate? >> ["[1]" ,"resource", ["1"]], # deprecate? >> ["[3.0]" ,"resource", ["3.0"]], # deprecate? >> >> #["true" ,"resource", ["true"]], literal true doesn't >> parse. true as a variable parses and creates a resource >> #["false" ,"resource", ["false"]], similar problems to >> [false] below. literal false doesn't parse >> ["[true]" ,"resource", ["true"]], # this makes no sense >> given the next case >> ["[false]" ,"error", /No title provided and :notify is >> not a valid resource reference/], # *sigh* >> >> #["undef" ,"resource", ["undef"]], # works for >> variable value, not for literal. deprecate? >> ["[undef]" ,"resource", ["undef"]], # deprecate? >> >> > > I'd say of the above that any value that works as a title when specified > indirectly via a variable should also work when specified as a literal, and > vise versa. I would be ok with requiring actual strings instead of > stringifying values of other types. I believe Henrik promoted that > approach. > > You are right. My "deprecate?" comments above where me wondering if the current parser should issue warnings. Now that I think about it, issuing warnings in these situations is impossible because the current parser doesn't make a clear distinction between numbers and strings. It is probably better to just rely on the new one failing when it encounters these situations. > > >> #["{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 And it works in a surprising number of situations. The title of the resource ends up being the hash: { "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 <- I haven't figured out what happened there other than using a hash as a title going awry). My test might have just been trying to find the resource incorrectly... > > >> >> ["/regexp/" ,"error", /Syntax error/], >> ["[/regexp/]" ,"error", /Syntax error/], >> >> ["default" ,"error", /Syntax error/], >> ["[default]" ,"error", /Syntax error/], >> >> The current system has a lot of odd outcomes in it. >> > > > I'd agree with that, but odd as they are, they are also relatively > consistent if you look at it from the right perspective. Mostly the > behavior springs from the fact that Puppet expects resource titles to be > strings, and from the fact that the grammar recognizes and distinguishes > the data types of many productions. There's a bit of a tension there, > though, because if you sneak a non-string individual resource title to the > back end by, say, hiding it in a variable, then the back end just > stringifies it instead of rejecting it. > > 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? > > >> Here are the tests for the future parser (as changed by the PR) >> >> ["thing" ,"resource", ["thing"]], >> ["[thing]" ,"resource", ["thing"]], >> ["[[nested, array]]" ,"resource", ["nested", "array"]], >> > > > I still find that last one surprising, but if it really matches the > current behavior then so be it. What's the rule here, though? Array > titles are automatically flattened? > > Right. Array titles are flattened and the flattened form is checked for duplicates. > > >> >> ["1" ,"error", /Illegal title type.*Expected >> String, got Integer/], >> ["3.0" ,"error", /Illegal title type.*Expected >> String, got Float/], >> ["[1]" ,"error", /Illegal title type.*Expected >> String, got Integer/], >> ["[3.0]" ,"error", /Illegal title type.*Expected >> String, got Float/], >> >> ["true" ,"error", /Illegal title type.*Expected >> String, got Boolean/], >> ["false" ,"error", /Illegal title type.*Expected >> String, got Boolean/], >> ["[true]" ,"error", /Illegal title type.*Expected >> String, got Boolean/], >> ["[false]" ,"error", /Illegal title type.*Expected >> String, got Boolean/], >> >> ["undef" ,"error", /Missing title.*undef/], >> ["[undef]" ,"error", /Missing title.*undef/], >> >> ["{nested => hash}" ,"error", /Illegal title type.*Expected >> String, got Hash/], >> ["[{nested => hash}]" ,"error", /Illegal title type.*Expected >> String, got Hash/], >> >> ["/regexp/" ,"error", /Illegal title type.*Expected >> String, got Regexp/], >> ["[/regexp/]" ,"error", /Illegal title type.*Expected >> String, got Regexp/], >> >> ["default" ,"resource", []], # nothing created because >> this is just a local default >> ["[default]" ,"resource", []], >> >> I'm a little worried that these changes might be too extreme. Thoughts? >> >> > > Beyond what I've already said, I don't personally have a problem with any > of the changes highlighted here. > > > 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/76d12bc2-5917-42b9-b397-c5773df8b9fe%40googlegroups.com > <https://groups.google.com/d/msgid/puppet-dev/76d12bc2-5917-42b9-b397-c5773df8b9fe%40googlegroups.com?utm_medium=email&utm_source=footer> > . > > For more options, visit https://groups.google.com/d/optout. > -- Andrew Parker a...@puppetlabs.com Freenode: zaphod42 Twitter: @aparker42 Software Developer *Join us at PuppetConf 2014 <http://www.puppetconf.com/>, September 22-24 in San Francisco* *Register by May 30th to take advantage of the Early Adopter discount <http://links.puppetlabs.com/puppetconf-early-adopter> **—**save $349!* -- 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/CANhgQXtL%3DWhMiyhj72ws3HUGJFGzmmYgrragR4Asrp_r4QUTpQ%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
