[08:47:01] jbond: morning sir, myself & fabfur are a little bit puzzled with the PCC output for https://gerrit.wikimedia.org/r/c/operations/puppet/+/938002/ || https://puppet-compiler.wmflabs.org/output/938002/42470/. From the CR you can see that it's basically a rename from hieradata/hosts/cp5017.yaml to hieradata/eqsin/profile/cache/haproxy.yaml, I was expecting a NOOP for cp5017 but it isn't the case [08:47:59] it looks like the merge with hieradata/common/profile/cache/haproxy.yaml isn't working as expected [09:04:01] vgutierrez: hieradta/$site/profile is part of the expand_paths hiera lookup a such it will only look for keys that are prefixed `profile::cache::haproxy::` in that file. anything elses will be ignored. specifically in this case the lookuip options will be ignored [09:04:21] jbond: all the content is profile::cache::haproxy:: [09:04:32] if possible i would just put the lookup options in common.yaml. if thats not dooable let me know and i can think of somthing elses [09:04:38] vgutierrez: lookup_options is not [09:04:43] jbond: lol [09:04:52] totally missed that :) [09:04:58] :) [09:06:47] fabfur: ^^ let's move those lookup_options to common.yaml :) [09:07:02] 🤞 [09:11:07] <_joe_> vgutierrez: ah I see john already told you [09:11:42] <_joe_> but tbh I would avoid the common hiera namespace as much as possible [09:11:56] <_joe_> just use the role/eqsin/... hierarchy [09:12:12] <_joe_> and refer to the roles you want to put stuff in [09:12:39] Can I use `hieradata/eqsin/common.yaml` for the lookup_options? [09:13:28] _joe_: we got a lot of stuff that's exactly the same for upload && text, like haproxy config [09:13:51] fabfur: the site equivilent would be hierdata/eqsin.yaml [09:14:19] jbond: ack [09:14:20] fabfur: considering that we have the very same lookup_options in hosts on other sites I'd go with common.yaml TBH [09:14:27] <_joe_> vgutierrez: repetition isn't always bad [09:14:42] <_joe_> and putting stuff in common.yaml non-namespaced is BAD [09:15:08] <_joe_> why is that hiera key called lookup_options ?? [09:15:11] _joe_: we are just taking about adding lookup_options to common.yaml [09:15:11] <_joe_> that's wrong [09:15:20] thats part of hiera [09:15:21] <_joe_> jbond: yeah I'm taking a step back [09:15:43] <_joe_> omg are they using the lookup options merge strategies [09:15:51] <_joe_> something I hoped we'd never ever use [09:16:11] <_joe_> I didn't think you could define them within hiera though [09:16:35] we've been defining them within hiera for a while [09:16:44] if using them it makes much more senses to define them in hiera to ensure that every lookup uses the same stradagy [09:18:25] <_joe_> and no, definitely NOT in common.,yaml [09:18:34] <_joe_> not without extended study [09:18:42] _joe_: so we rollback the lines that have been there for 2 years already? [09:19:20] it is not like we (traffc) are introducing anything new [09:19:22] <_joe_> vgutierrez: I am saying we can't add to common.yaml a lookup strategy that wil lchange how all of hiera will be looked up [09:19:24] <_joe_> without testing [09:19:46] _joe_: its not every key its a per key oreride [09:19:53] look at the top of common.yaml [09:20:02] we are impacting very specific profile::cache::haproxy hiera keys [09:20:22] <_joe_> vgutierrez: I know, I hoped we didn't have lookup_options in common.yaml [09:20:57] <_joe_> it makes how things work for a specific role hard to understand [09:21:20] <_joe_> jbond: we don't allow module data still, right? because that's where I'd find natural to put such stuff [09:21:55] the idea here is to avoid duplicating hiera keys while we perform a per DC roll out, as soon as everything is ready we can get rid of those [09:22:12] _joe_: it works, its used in one module netbase [09:22:38] in the past we had issues with people forgetting to update hieradata properly due to duplicity, so this minimizes the chances of that from happening [09:22:45] and i did have a patch top move profile to in module data which i think yu reviewed. but im not sure about it as its a big change to work flows [09:22:51] would definetly need to be communicated well [09:23:06] <_joe_> jbond: so my order of preference is 1) define lookup_options in the lookup function in the class [09:23:10] <_joe_> which you can do [09:23:17] <_joe_> 2) do it as module data [09:23:57] for me one is the worst option as every caller that wants to use that variable needs to know which lookup stratagy is ment to be used [09:23:58] <_joe_> jbond: if they moved the lookup_options in the lookup() call for the parameter, it would work everywhere, and be clear from the code [09:24:23] <_joe_> you are thinking hiera is the driver, I think it's the other way around [09:24:41] <_joe_> it's the code that defines the data structures to use and how it's going to use them [09:25:12] yes i hat the use of the lookup optoin altogether and thinkg we should use automatic paramter looukups (i know we differ here so lets not tangent ;)) [09:25:27] <_joe_> sorry, need to go afk right now [09:25:34] <_joe_> go on with what you've done up to now [09:25:36] <_joe_> :) [09:26:48] adding them to the specific lookup() calls works for us and being the only use case for Traffic I don't have an issue [09:26:56] * jbond meant hate the use of the the lookup function above [09:27:20] but I think we should document the canonical way in https://wikitech.wikimedia.org/wiki/Puppet/Coding_and_style_guidelines [09:30:48] vgutierrez: personaly i would say at this point its better to have it in common.yaml for consitency. i dont see any usecase in the current y repo passing strategy to the lookup call [09:31:12] makes sense IMHO [23:04:40] I will be back by Halloween:)