Issue 4810150141493248: Issue 122 - Puppet ENC via Hiera (Closed)
|
Created:
Sept. 8, 2014, 11:56 a.m. by mathias Modified:
March 17, 2015, 4:28 a.m. CC:
aalvz, paco Visibility:
Public. |
DescriptionThis is a proof of concept published as a request for comments. It is NOT proposed to be pushed/merged yet.
Patch Set 1 #
Total comments: 24
Patch Set 2 : 112 - Puppet ENC via Hiera #
Total comments: 31
Patch Set 3 : 112 - Puppet ENC via Hiera #
Total comments: 1
Patch Set 4 : 112 - Integrate run.py and monitoring with Hiera #Patch Set 5 : 112 - Integrate run.py and monitoring with Hiera #
Total comments: 52
Patch Set 6 : Puppet ENC via Hiera #
Total comments: 2
Patch Set 7 : Puppet ENC via Hiera #
Total comments: 33
Patch Set 8 : Puppet ENC via Hiera #Patch Set 9 : Puppet ENC via Hiera - Without Arrow Alignment #
MessagesTotal messages: 31
Looks promising, the most serious issue IMHO is requiring one YAML file per server rather than one YAML file per server type. Btw, I recommend configuring the following in your .hgrc file: [diff] git = true This will make sure that there is a diff for binary files as well. http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/hier... File hiera/environments/production (right): http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/hier... hiera/environments/production:1: ../../modules/private/hiera Is that a symlink? I'm not sure whether it is a good idea to commit those to Mercurial, I would be surprised if they still work correctly on Windows. http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/hier... File hiera/files/classify.sh (right): http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/hier... hiera/files/classify.sh:7: | ruby -ryaml Wow, that's quite a hack... Why use a shell script to generate a Ruby script dynamically instead of a static Ruby script that will simply get the data from Hiera and convert it to YAML? That should remove the potential security issues here. Note that ideally you wouldn't run Hiera as a separate process (according to https://projects.puppetlabs.com/issues/15183 the output isn't meant for anything other than humans) but rather use the example under https://github.com/puppetlabs/hiera/blob/master/README.md#querying-from-code. http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/hier... File hiera/files/precise.sh (right): http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/hier... hiera/files/precise.sh:20: fi I think you already know my opinion on using shell scripts for anything beyond the most trivial things :) It definitely looks like this would be better off as a Python script running with root privileges (pretty much every operation here requires root privileges anyway). http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/hier... hiera/files/precise.sh:28: ln -fs puppet/hiera.yaml /etc/hiera.yaml Doesn't this need root privileges as well? http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/hier... hiera/files/precise.sh:32: ln -fs /vagrant/hiera/files/classify.sh /usr/local/bin/puppet-node-classifier So puppet-node-classifier is actually our classify.sh? That's pretty misleading IMHO, the name should reflect that this isn't some default command but rather our script. Actually, what's wrong with running it from /vagrant/hiera/files/classify.sh? Same here btw, doesn't this need root privileges? http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/hier... File hiera/vagrant.yaml (right): http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/hier... hiera/vagrant.yaml:9: - "nodes/%{::hostname}" That's the wrong approach - we have YAML files per server type, not for individual servers. So for example server codereview1 has type codereview, meaning that nodes/codereview.yaml should be applied for it. Otherwise we would need a separate YAML file for each filter server. http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/modu... File modules/base/manifests/init.pp (right): http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/modu... modules/base/manifests/init.pp:47: host{"$name,$ip": Will this work correctly for hosts that have multiple IP addresses? http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/modu... modules/base/manifests/init.pp:50: name => "$name.adblockplus.org", Unfortunately, this isn't all that simple right now - some servers have names that don't match their DNS name. For example, server4 is listed as server_4 in DNS. So an additional (optional?) DNS name needs to be specified in hosts.yaml. http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/modu... modules/base/manifests/init.pp:56: type => ssh-rsa, Not sure but I guess that the IP addresses of the host need to be specified as host_aliases. http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/modu... File modules/filtermaster/manifests/init.pp (right): http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:6: include ssh Isn't it already included by the base module? http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/modu... File modules/private (right): http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/modu... modules/private:1: private-stub Here as well, I don't think that committing symbolic links is a good idea. http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/modu... File modules/statsclient/manifests/init.pp (right): http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/modu... modules/statsclient/manifests/init.pp:3: include ssh Isn't it already included by the base module?
http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/hier... File hiera/environments/production (right): http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/hier... hiera/environments/production:1: ../../modules/private/hiera On 2014/09/09 18:15:06, Wladimir Palant wrote: > Is that a symlink? I'm not sure whether it is a good idea to commit those to > Mercurial, I would be surprised if they still work correctly on Windows. Actually this was included in the patch-set by accident and is to be removed in the next one. http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/hier... File hiera/files/classify.sh (right): http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/hier... hiera/files/classify.sh:7: | ruby -ryaml On 2014/09/09 18:15:06, Wladimir Palant wrote: > Wow, that's quite a hack... > > Why use a shell script to generate a Ruby script dynamically instead of a static > Ruby script that will simply get the data from Hiera and convert it to YAML? > That should remove the potential security issues here. > > Note that ideally you wouldn't run Hiera as a separate process (according to > https://projects.puppetlabs.com/issues/15183 the output isn't meant for anything > other than humans) but rather use the example under > https://github.com/puppetlabs/hiera/blob/master/README.md#querying-from-code. Of course it's to be converted - this one here was intended for the proof-of-concept only. A way more sophisticated version in Ruby will be proposed in the next patch-set. http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/hier... File hiera/files/precise.sh (right): http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/hier... hiera/files/precise.sh:20: fi On 2014/09/09 18:15:06, Wladimir Palant wrote: > I think you already know my opinion on using shell scripts for anything beyond > the most trivial things :) > > It definitely looks like this would be better off as a Python script running > with root privileges (pretty much every operation here requires root privileges > anyway). Indeed, I know about your preferences, and I know that it's common practice to avoid shell-scripts for non-trivial tasks. Still, since this is pretty straightforward, short, does not accept any user input, and is applicable in development only - how does this not classify as trivial? Anyway, the next patch-set will feature an improved version in Python removing the need to track additional files such as puppetlabs.list etc. and also begin applicable in other environments. http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/hier... hiera/files/precise.sh:28: ln -fs puppet/hiera.yaml /etc/hiera.yaml On 2014/09/09 18:15:06, Wladimir Palant wrote: > Doesn't this need root privileges as well? Yes it does. However, with the conversion of classify.sh to puppet-node-classifier.rb this link is not required any more (we use /etc/puppet/hiera there). http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/hier... hiera/files/precise.sh:32: ln -fs /vagrant/hiera/files/classify.sh /usr/local/bin/puppet-node-classifier On 2014/09/09 18:15:06, Wladimir Palant wrote: > So puppet-node-classifier is actually our classify.sh? That's pretty misleading > IMHO, the name should reflect that this isn't some default command but rather > our script. Actually, what's wrong with running it from > /vagrant/hiera/files/classify.sh? > > Same here btw, doesn't this need root privileges? Agreed, there's no need to "install" the script in the first place, which - of course - applies to the new Ruby version as well. http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/hier... File hiera/vagrant.yaml (right): http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/hier... hiera/vagrant.yaml:9: - "nodes/%{::hostname}" On 2014/09/09 18:15:06, Wladimir Palant wrote: > That's the wrong approach - we have YAML files per server type, not for > individual servers. So for example server codereview1 has type codereview, > meaning that nodes/codereview.yaml should be applied for it. Otherwise we would > need a separate YAML file for each filter server. Well, actually this is not wrong. Having separate or linked specs per host is quite common, especially when adjusting to heterogeneous infrastructure. However, we where planning on integrating roles resp. types anyway (and have already done so, see next patch-set), so that host-specific stuff is still possible but not used if not required. http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/modu... File modules/base/manifests/init.pp (right): http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/modu... modules/base/manifests/init.pp:47: host{"$name,$ip": On 2014/09/09 18:15:06, Wladimir Palant wrote: > Will this work correctly for hosts that have multiple IP addresses? It will, although the host{$key:} looks awkward then: Puppet does not insert spaces or commas between the array items. The $ip parameter works fine anyway. However, we'll improve that one. http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/modu... modules/base/manifests/init.pp:50: name => "$name.adblockplus.org", On 2014/09/09 18:15:06, Wladimir Palant wrote: > Unfortunately, this isn't all that simple right now - some servers have names > that don't match their DNS name. For example, server4 is listed as server_4 in > DNS. So an additional (optional?) DNS name needs to be specified in hosts.yaml. Done. http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/modu... modules/base/manifests/init.pp:56: type => ssh-rsa, On 2014/09/09 18:15:06, Wladimir Palant wrote: > Not sure but I guess that the IP addresses of the host need to be specified as > host_aliases. Done. http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/modu... File modules/filtermaster/manifests/init.pp (right): http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:6: include ssh On 2014/09/09 18:15:06, Wladimir Palant wrote: > Isn't it already included by the base module? It is, but that does not ensure that the base/ssh stuff for the concat module has been done before concat::fragment is invoked from this file. http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/modu... File modules/private (right): http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/modu... modules/private:1: private-stub On 2014/09/09 18:15:06, Wladimir Palant wrote: > Here as well, I don't think that committing symbolic links is a good idea. This was an accidental commit that is already removed it in the development branch, will be gone in the next patch-set. http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/modu... File modules/statsclient/manifests/init.pp (right): http://codereview.adblockplus.org/4810150141493248/diff/5629499534213120/modu... modules/statsclient/manifests/init.pp:3: include ssh On 2014/09/09 18:15:06, Wladimir Palant wrote: > Isn't it already included by the base module? It is, but that does not ensure that the base/ssh stuff for the concat module has been done before concat::fragment is invoked from this file.
http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/READ... File README.md (right): http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/READ... README.md:13: *development*, *test* and *production*), whilst others are confidential. Explicitly mention passwords as example of "confidential"? This leaves quite a bit room for interpretation otherwise, we don't want the impression that we are hiding something here (yes, I know that we sort of explain it below already). http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/Vagr... File Vagrantfile (right): http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/Vagr... Vagrantfile:27: if role == nil Can role ever be nil? I don't think we want to have host-specific configuration, all hosts are configured according to their role (which can be assigned to zero, one or many hosts). http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/Vagr... Vagrantfile:28: setup_path = File.join(Dir.pwd, "hiera", "hosts", "#{host_name}.yaml") Please don't use Dir.pwd here or elsewhere, its value is "random" - REPOSITORY_DIR should be used instead. http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/Vagr... Vagrantfile:55: '--debug', Debug output isn't going to be too useful, right? http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/hier... File hiera/development/hosts.yaml (right): http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/hier... hiera/development/hosts.yaml:4: ip: [10.8.0.120, '2a01:4f8:100:81a4::2'] I don't think production IPv6 addresses make sense here. We can assign IPv6 addresses to development servers of course - but then these IPv6 addresses should come from a non-routable subnet and actually be respected. http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/hier... hiera/development/hosts.yaml:5: ssh_public_key: AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBAUhHiFdqQBATyyOE9XPR7v2+bghp3r1QY0O62d/klV7yhAOBKQuI6MtTu43cYQE60M10OzVfeeMfmc0dPP97UM= role: filterserver? Similarly for the hosts below. http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/hier... File hiera/hiera.yaml (right): http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/hier... hiera/hiera.yaml:10: - "environment/hosts" Nit: If I understand this correctly, quotation marks are unnecessary in the two lines above. http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/hier... hiera/hiera.yaml:11: - common This shouldn't be necessary? http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/hier... File hiera/install-precise.py (right): http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/hier... hiera/install-precise.py:45: B2pi+8u84f+an4Hml4xlyijgYu05pqNvnLRyJDLd61hviLC8GYWwAgAD''' https://pgp.mit.edu/pks/lookup?op=get&search=0x1054B7A24BD6EC30 is the same key, right? Why not copy the text from there and import it as-is, without doing base64 decoding yourself? That's actually what we discussed here... http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/hier... hiera/install-precise.py:47: handle = subprocess.Popen(['apt-key', 'add', '-'], stdin=subprocess.PIPE) I was already considering asking for that handle to be closed when I realized that this isn't actually a handle - it's a process object ;) Maybe use a better variable name? http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/hier... hiera/install-precise.py:62: config = os.path.dirname(__file__) + '/hiera.yaml' Please use os.path.join() instead of string concatenation here. http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/hier... File hiera/puppet-node-classifier.rb (right): http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/hier... hiera/puppet-node-classifier.rb:13: HOSTS_CONFIG = ENV.fetch('PUPPET_HOSTS_CONFIG', '/etc/puppet/infrastructure/hiera/environment/hosts.yaml') I don't think hardcoding /etc/puppet/infrastructure/hiera here is necessary, File.dirname(__FILE__) should do? http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/modu... File modules/base/manifests/init.pp (right): http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/modu... modules/base/manifests/init.pp:56: host{"$name,$internal_ip": Given that each name is resolved to a single IP address, host{$name: should do just fine. http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/modu... File modules/filtermaster/manifests/init.pp (right): http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/modu... modules/filtermaster/manifests/init.pp:6: include ssh That change shouldn't be necessary, is that a rebase artifact? http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/modu... File modules/statsclient/manifests/init.pp (right): http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/modu... modules/statsclient/manifests/init.pp:3: include ssh That change shouldn't be necessary, is that a rebase artifact? http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/modu... File modules/statsmaster/manifests/init.pp (left): http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/modu... modules/statsmaster/manifests/init.pp:38: source => 'puppet:///modules/statsmaster/known_hosts', Remove that file now that it isn't needed any more?
http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/READ... File README.md (right): http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/READ... README.md:13: *development*, *test* and *production*), whilst others are confidential. On 2014/11/17 16:43:36, Wladimir Palant wrote: > Explicitly mention passwords as example of "confidential"? This leaves quite a > bit room for interpretation otherwise, we don't want the impression that we are > hiding something here (yes, I know that we sort of explain it below already). Done. http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/Vagr... File Vagrantfile (right): http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/Vagr... Vagrantfile:27: if role == nil On 2014/11/17 16:43:36, Wladimir Palant wrote: > Can role ever be nil? I don't think we want to have host-specific configuration, > all hosts are configured according to their role (which can be assigned to zero, > one or many hosts). Agreed, we do not need a host being configured by itself (though I still see a use for additional, host-specific setup - especially when different server hardware operates in a cluster). Note, however, that due to various limitations (in Ruby, Puppet and Hiera itself) it is not trivial at all to properly merge multiple roles into each other - which is why only "zero" or "one" role can be assigned - multiple ones have to be combined by creating another role. http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/Vagr... Vagrantfile:28: setup_path = File.join(Dir.pwd, "hiera", "hosts", "#{host_name}.yaml") On 2014/11/17 16:43:36, Wladimir Palant wrote: > Please don't use Dir.pwd here or elsewhere, its value is "random" - > REPOSITORY_DIR should be used instead. Done. http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/Vagr... Vagrantfile:55: '--debug', On 2014/11/17 16:43:36, Wladimir Palant wrote: > Debug output isn't going to be too useful, right? It is super useful! It doesn't seem to make much sense running Puppet from Vagrant without the debug option: When something fails during development or test, one may not be able to determine the exact reason without the debug output and thus be forced to re-play the entire scenario (which may not be an exact replay as some states may have changed already etc). In fact it happens so often - especially when some ordering went wrong - that one gets annoyed by not having the debug output in the first place. Also there's no guarantee unordered parts are executed the same way again. To make a long story short: I am absolutely sure we (read "Alfonso, Paco and Matze, at least") do want the option set here by default. And no, the verbose option alone is, in many cases, not enough.. http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/hier... File hiera/development/hosts.yaml (right): http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/hier... hiera/development/hosts.yaml:4: ip: [10.8.0.120, '2a01:4f8:100:81a4::2'] On 2014/11/17 16:43:36, Wladimir Palant wrote: > I don't think production IPv6 addresses make sense here. We can assign IPv6 > addresses to development servers of course - but then these IPv6 addresses > should come from a non-routable subnet and actually be respected. Done. http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/hier... hiera/development/hosts.yaml:5: ssh_public_key: AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBAUhHiFdqQBATyyOE9XPR7v2+bghp3r1QY0O62d/klV7yhAOBKQuI6MtTu43cYQE60M10OzVfeeMfmc0dPP97UM= On 2014/11/17 16:43:36, Wladimir Palant wrote: > role: filterserver? Similarly for the hosts below. Sure, but not until we actually port the respective server setup to the ENC and create the required role. http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/hier... File hiera/hiera.yaml (right): http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/hier... hiera/hiera.yaml:10: - "environment/hosts" On 2014/11/17 16:43:36, Wladimir Palant wrote: > Nit: If I understand this correctly, quotation marks are unnecessary in the two > lines above. Sure they are, but syntax highlighting is more intuitive this way. http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/hier... hiera/hiera.yaml:11: - common On 2014/11/17 16:43:36, Wladimir Palant wrote: > This shouldn't be necessary? Indeed, it is not. It's an alias for various, system-specific standard paths recommended to be at the end of the list. This e.g. allows for you to add some specific files on the puppet master without changing the repo or the symlinks in there. However, we remove it until it may become necessary. http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/hier... File hiera/install-precise.py (right): http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/hier... hiera/install-precise.py:45: B2pi+8u84f+an4Hml4xlyijgYu05pqNvnLRyJDLd61hviLC8GYWwAgAD''' On 2014/11/17 16:43:36, Wladimir Palant wrote: > https://pgp.mit.edu/pks/lookup?op=get&search=0x1054B7A24BD6EC30 is the same key, > right? Why not copy the text from there and import it as-is, without doing > base64 decoding yourself? That's actually what we discussed here... In fact I do not recall us discussing to use the stuff from that URL explicitly, though I remember us discussing the possibility for something like that. However, the link above is not the same key (it's for YUM and TAR.GZ, not DEB), we'll use http://apt.puppetlabs.com/pubkey.gpg http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/hier... hiera/install-precise.py:47: handle = subprocess.Popen(['apt-key', 'add', '-'], stdin=subprocess.PIPE) On 2014/11/17 16:43:36, Wladimir Palant wrote: > I was already considering asking for that handle to be closed when I realized > that this isn't actually a handle - it's a process object ;) > > Maybe use a better variable name? Done. http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/hier... hiera/install-precise.py:62: config = os.path.dirname(__file__) + '/hiera.yaml' On 2014/11/17 16:43:36, Wladimir Palant wrote: > Please use os.path.join() instead of string concatenation here. Done. http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/hier... File hiera/puppet-node-classifier.rb (right): http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/hier... hiera/puppet-node-classifier.rb:13: HOSTS_CONFIG = ENV.fetch('PUPPET_HOSTS_CONFIG', '/etc/puppet/infrastructure/hiera/environment/hosts.yaml') On 2014/11/17 16:43:36, Wladimir Palant wrote: > I don't think hardcoding /etc/puppet/infrastructure/hiera here is necessary, > File.dirname(__FILE__) should do? It would work in this case, indeed. Yet I don't believe it's a good idea to operate with paths relative to some script, especially if the desired files are not siblings in the very same folder (but e.g. in sub-directories, as in this case). The POSIX way of assuming some default location in /etc seems to be more "clean", and one could consider the /etc/puppet/infrastructure path as the canonical configuration path within our flavor of Puppet - especially since we're using the absolute path's in related resources as well (kick.py and install-precise.py, for example). http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/modu... File modules/base/manifests/init.pp (right): http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/modu... modules/base/manifests/init.pp:56: host{"$name,$internal_ip": On 2014/11/17 16:43:36, Wladimir Palant wrote: > Given that each name is resolved to a single IP address, host{$name: should do > just fine. Done. http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/modu... File modules/filtermaster/manifests/init.pp (right): http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/modu... modules/filtermaster/manifests/init.pp:6: include ssh On 2014/11/17 16:43:36, Wladimir Palant wrote: > That change shouldn't be necessary, is that a rebase artifact? No it is not, that one is actually intentional: tl;dr -- As denoted in the comments for patch set 1; the implicit inclusion of class `ssh` in `base` does not guarantee that it has been processed before concat::fragment for `sshd_max_limits` here. -vvv -- When using an ENC (doesn't have to be Hiera), one can use either a list of classes or a hash/map/dict (see https://docs.puppetlabs.com/guides/external_nodes.html#enc-output-format). Since we're merging our YAML role files with precedences over each other, we can't use the list version but have to rely on the maps (otherwise merging would become really complicated and prone to error). Unfortunately, this also implies that we cannot control in which order the classes are applied (due to hash maps not reflecting the insertion order) - in contrast to before, where this was determined implicitly by the include in the node's manifest. We have various ideas how to circumvent this in another fashion than the explicit include here. However, testing alternatives is something we're planning to do while migrating the affected classes to be integrated with ENC/Hiera. This include should be a fine solution for now, at least. http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/modu... File modules/statsclient/manifests/init.pp (right): http://codereview.adblockplus.org/4810150141493248/diff/5657382461898752/modu... modules/statsclient/manifests/init.pp:3: include ssh On 2014/11/17 16:43:36, Wladimir Palant wrote: > That change shouldn't be necessary, is that a rebase artifact? No it is not, that one is actually intentional. Please refer to my former comments above and for the first patch set.
http://codereview.adblockplus.org/4810150141493248/diff/5713144022302720/hier... File hiera/install-precise.py (right): http://codereview.adblockplus.org/4810150141493248/diff/5713144022302720/hier... hiera/install-precise.py:2: import base64 Just saw I forgot to remove this one; will be included in the next patch set (or push); see https://github.com/mjhennig/adblockplus-infrastructure/commit/607596448340b93...
LGTM
Now including the changes in modules/private-stub grmlslslsls!!!
On 2015/02/26 17:59:27, matze wrote: > Now including the changes in modules/private-stub grmlslslsls!!! hiera/puppet-node-classifier.rb hiera/install-precise.py need to be executable, I don't know why it doesn't preserve the mode when I download and apply the patch, otherwise it LGTM!
On 2015/02/26 18:17:09, paco wrote: > On 2015/02/26 17:59:27, matze wrote: > > Now including the changes in modules/private-stub grmlslslsls!!! > > hiera/puppet-node-classifier.rb > hiera/install-precise.py > > need to be executable, I don't know why it doesn't preserve the mode when I > download and apply the patch, otherwise it LGTM! Try `hg import ...` or `git apply ...` --
http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.py File run.py (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.... run.py:72: or name in item[1]['ip'] While resolving IPs would be a nice feature, this line here is not enough. It does not handle different phenotypes of the same IPv6 address, for example, and also does not reflect that we actually allow a single IP to be given as a string in `modules/base/manifests/init.pp`. Since the effort to integrate these facets is way too high for now, I'll remove that line in the next patch-set (RC2).
Since I'm a reviewer now, I took a look at all the changes. (Also added Wladimir back to CC, since he still might want to review the rest of this.) Did more of a high level review at this point, but it looks great all in all. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/READ... File README.md (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/READ... README.md:9: Host specific setup Is that really "Host specific"? You're calling servers "hosts" below, so IMO this is a bit confusing. Maybe something like "Environment specific setup"? Or just "Private files" or maybe "Private resources"? :) http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/READ... README.md:25: # UNIX-oid Why the "-old" here? Also, I would go for something like a #### headline instead of code comments here. Also because "#" doesn't introduce a comment in batch scripts, so it doesn't really make sense for the Windows part below. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/READ... README.md:26: user@host:~/infrastructure$ ln -s private-stub modules/private I'd opt for just having `$ ln -s private-stub modules/private` - the rest of the prompt doesn't matter, and it's quite customary to leave it out. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/READ... README.md:61: ### Requirements Shouldn't we also add hiera here? We should also describe the PUPPET_HIERA_CONFIG and PUPPET_HOSTS_CONFIG variables. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/READ... README.md:113: Configuring Puppet This part of "Adding a host", so it should rather be a ### headline I'd say. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/READ... README.md:116: Below please find brief instructions for setting up Puppet on both master Sounds awfully formal and doesn't really add anything IMO, I'd just get rid of this paragraph :) http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/Vagr... File Vagrantfile (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/Vagr... Vagrantfile:65: configYAML = File.join(REPOSITORY_DIR, "hiera/private/hosts.yaml") We're going with underscore rather than camel case in the rest of this file (and in Ruby code in general), there's more camel case below. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/hier... File hiera/install-precise.py (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/hier... hiera/install-precise.py:1: #!/usr/bin/env python Nit: New line after the hash bang line? http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/hier... hiera/install-precise.py:86: add_key_process = subprocess.Popen(['apt-key', 'add', '-'], stdin=subprocess.PIPE) All of this stuff looks like it'll work on any system using APT - not just Ubuntu, and not just precise either. Maybe we should call the script "install_apt.py" or something? http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/modu... File modules/base/manifests/init.pp (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/modu... modules/base/manifests/init.pp:1: class base ($authority='adblockplus.org') { Wouldn't "zone" or maybe "domain" be a more fitting name here? http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.py File run.py (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.... run.py:6: import os.path Not necessary, we're already importing os. Looks like we're actually only using os.path here so we could get rid of the `import os`, however, that's an unrelated change that should better go to a separate (Noissue) commit. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.... run.py:60: valid_hosts = getValidHosts() Host validation doesn't make _that_ much sense anymore now that the development infrastructure doesn't have to mirror the production infrastructure anymore. That'd only work if we work with the real infrastructure-private rather than private-stub locally, which is probably not a good idea. How about relying on us sticking to our naming scheme consistently and just checking that there's a role for that server name? For example, we'd check that there's a "downloadserver" role when the user wants to run something on a server called "download\d".
Frankly, this is far beyond the point where I can still give it a "looks good to me" - I lost overview a few iterations ago. Let's hope that Felix can still review this properly. As far as I am concerned, I'm seriously wondering whether you should just start over and get there in smaller steps and without unrelated changes (yes, it is possible). http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/READ... File README.md (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/READ... README.md:25: # UNIX-oid On 2015/03/03 16:43:32, Felix H. Dahlke wrote: > Why the "-old" here? I guess using "Unix-like" will avoid that confusion... http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/hier... File hiera/puppet-node-classifier.rb (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/hier... hiera/puppet-node-classifier.rb:86: result = { 'classes'=> classes, 'parameters'=> parameters } Nit: Space before => please. Same for scope object above. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/hier... File hiera/roles/monitoringserver.yaml (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/hier... hiera/roles/monitoringserver.yaml:24: members: '' Is this "TODO"? http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/modu... File modules/base/manifests/init.pp (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/modu... modules/base/manifests/init.pp:51: $groups = undef, This parameter is unused, why did you add it? http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/modu... File modules/nagios/manifests/server.pp (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/modu... modules/nagios/manifests/server.pp:127: <- This change seems bogus: hosts.cfg can be written independently of hostgroups.cfg. Both require nagios3 package however, the directory won't exist otherwise. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.py File run.py (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.... run.py:56: host_names = set(str(item) for item in hosts) Given that hosts is a list of string, this seems equivalent to |host_names = set(hosts)|. However, I don't see why we should convert it to a set at all. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.... run.py:60: valid_hosts = getValidHosts() On 2015/03/03 16:43:32, Felix H. Dahlke wrote: > Host validation doesn't make _that_ much sense anymore now that the development > infrastructure doesn't have to mirror the production infrastructure anymore. > That'd only work if we work with the real infrastructure-private rather than > private-stub locally, which is probably not a good idea. Indeed, that's something we discussed previously - so I'm rather surprised to see parsing of the local hosts.yaml here. The idea we discussed was rather getting the list of hosts from the puppet master. So we would update the repositories on the puppet master first, ask it for its hosts.yaml (something along the lines of "ssh puppetmaster cat /etc/puppet/infrastructure/hiera/private/hosts.yaml" should do) and do host validation based on that list. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.... run.py:67: item[1].get('dns', item[0]) for item in valid_hosts.items() for (key, value) in valid_hosts.iteritems() please - destructure the tuple immediately, otherwise this is unreadable. Also, why tuple() if this really a list? chunk = [ value.get('dns', key) for (key, value) in valid_hosts.iteritems() ... ]
RC2, as announced.
Loosing overview is what happens when branches live too long.. ;) Seriously, starting over does not have much of an effect despite requiring even more time. Also, I do not see much "unrelated" changes. However, I would suggest finishing the discussion of the points in this review here first. Then I can re-write the history to become more straightforward, drop the migration of codereviewserver.pp and intraforumserver.pp for now (anything else is rather mandatory for a clean merge) and start over with a new review issue. This can be done in one or two hours after the aforementioned points have been addressed. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/READ... File README.md (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/READ... README.md:9: Host specific setup On 2015/03/03 16:43:32, Felix H. Dahlke wrote: > Is that really "Host specific"? You're calling servers "hosts" below, so IMO > this is a bit confusing. Maybe something like "Environment specific setup"? Or > just "Private files" or maybe "Private resources"? :) Done. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/READ... README.md:25: # UNIX-oid On 2015/03/03 16:43:32, Felix H. Dahlke wrote: > Why the "-old" here? > > Also, I would go for something like a #### headline instead of code comments > here. Also because "#" doesn't introduce a comment in batch scripts, so it > doesn't really make sense for the Windows part below. Done. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/READ... README.md:26: user@host:~/infrastructure$ ln -s private-stub modules/private On 2015/03/03 16:43:32, Felix H. Dahlke wrote: > I'd opt for just having `$ ln -s private-stub modules/private` - the rest of the > prompt doesn't matter, and it's quite customary to leave it out. Done. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/READ... README.md:61: ### Requirements On 2015/03/03 16:43:32, Felix H. Dahlke wrote: > Shouldn't we also add hiera here? We should also describe the > PUPPET_HIERA_CONFIG and PUPPET_HOSTS_CONFIG variables. Yes we should improve on documentation, yet this is not the right place do document either Hiera or Puppet. These are the requirements you need to set up a development environment. And in fact one does not need Puppet or Hiera for that purpose - they are only used in the guest boxes. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/READ... README.md:113: Configuring Puppet On 2015/03/03 16:43:32, Felix H. Dahlke wrote: > This part of "Adding a host", so it should rather be a ### headline I'd say. Not exclusively, it also applies to the master. Nonetheless I've extended the above section, and updated this one. That should improve it in the sense of your comment. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/READ... README.md:116: Below please find brief instructions for setting up Puppet on both master On 2015/03/03 16:43:32, Felix H. Dahlke wrote: > Sounds awfully formal and doesn't really add anything IMO, I'd just get rid of > this paragraph :) Done. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/Vagr... File Vagrantfile (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/Vagr... Vagrantfile:65: configYAML = File.join(REPOSITORY_DIR, "hiera/private/hosts.yaml") On 2015/03/03 16:43:32, Felix H. Dahlke wrote: > We're going with underscore rather than camel case in the rest of this file (and > in Ruby code in general), there's more camel case below. Done. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/hier... File hiera/install-precise.py (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/hier... hiera/install-precise.py:1: #!/usr/bin/env python On 2015/03/03 16:43:32, Felix H. Dahlke wrote: > Nit: New line after the hash bang line? Done. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/hier... hiera/install-precise.py:86: add_key_process = subprocess.Popen(['apt-key', 'add', '-'], stdin=subprocess.PIPE) On 2015/03/03 16:43:32, Felix H. Dahlke wrote: > All of this stuff looks like it'll work on any system using APT - not just > Ubuntu, and not just precise either. Maybe we should call the script > "install_apt.py" or something? If we do not actively develop for Debian, we cannot call it *-apt or *-debian. Many derivatives maintain their own APT resources, with specific versions for each package available. Ubuntu, in particular, tends to have more recent but less stable versions. So just because it works on our boxes it does not mean it works with APT-driven OS in general. IMHO we should be precise here (pun not intended) and only indicate compatibility with the systems we've actually tested that stuff on. However, just as an FYI, I can confirm that this actually works with Debian 7 - the only other system I've tested so far, and that was just by accident. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/hier... File hiera/puppet-node-classifier.rb (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/hier... hiera/puppet-node-classifier.rb:86: result = { 'classes'=> classes, 'parameters'=> parameters } On 2015/03/03 20:00:19, Wladimir Palant wrote: > Nit: Space before => please. Same for scope object above. Done. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/hier... File hiera/roles/monitoringserver.yaml (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/hier... hiera/roles/monitoringserver.yaml:24: members: '' On 2015/03/03 20:00:19, Wladimir Palant wrote: > Is this "TODO"? Nope. The servers are added into the groups already (see hosts.yaml and module nagios). Though the groups have to be defined somewhere, which is done here. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/modu... File modules/base/manifests/init.pp (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/modu... modules/base/manifests/init.pp:1: class base ($authority='adblockplus.org') { On 2015/03/03 16:43:32, Felix H. Dahlke wrote: > Wouldn't "zone" or maybe "domain" be a more fitting name here? Indeed, let's go with $zone. $domain is much to over-used and not precise enough anyway. Which is why we've tried to avoid it here in the first place. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/modu... modules/base/manifests/init.pp:51: $groups = undef, On 2015/03/03 20:00:19, Wladimir Palant wrote: > This parameter is unused, why did you add it? It's one of the allowed keywords in hosts.yaml, and used to accumulate e.g. Nagios host-groups in modules/nagios/manifests/server.pp -- Unfortunately, create_resources() requires all key/value pairs to contain only keys recognized by the resource type. Having unused parameters when processing the data is the only way to introduce some flexibility here (without writing Ruby code, at least). http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/modu... File modules/nagios/manifests/server.pp (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/modu... modules/nagios/manifests/server.pp:127: <- On 2015/03/03 20:00:19, Wladimir Palant wrote: > This change seems bogus: hosts.cfg can be written independently of > hostgroups.cfg. Both require nagios3 package however, the directory won't exist > otherwise. You're right regarding Nagios, but it's actually Puppet that caused us to do so: When using the nagios_host {..: groups => } parameter, it requires the group to be defined already. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.py File run.py (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.... run.py:6: import os.path On 2015/03/03 16:43:32, Felix H. Dahlke wrote: > Not necessary, we're already importing os. Looks like we're actually only using > os.path here so we could get rid of the `import os`, however, that's an > unrelated change that should better go to a separate (Noissue) commit. Agreed, the explicit os.path import will be removed in RC2. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.... run.py:56: host_names = set(str(item) for item in hosts) On 2015/03/03 20:00:19, Wladimir Palant wrote: > Given that hosts is a list of string, this seems equivalent to |host_names = > set(hosts)|. However, I don't see why we should convert it to a set at all. "Given that hosts is a list of string" <- this is an assumption; inside the function there is no guarantee for that one. You know already that I don't like dropping good practices without a good reason. But I do agree that there is no need for host_names to be a set, since uniqueness is ensured in the result set already. That also means it can remain a generator. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.... run.py:60: valid_hosts = getValidHosts() On 2015/03/03 16:43:32, Felix H. Dahlke wrote: > Host validation doesn't make _that_ much sense anymore now that the development > infrastructure doesn't have to mirror the production infrastructure anymore. > That'd only work if we work with the real infrastructure-private rather than > private-stub locally, which is probably not a good idea. > > How about relying on us sticking to our naming scheme consistently and just > checking that there's a role for that server name? For example, we'd check that > there's a "downloadserver" role when the user wants to run something on a server > called "download\d". This version does two jobs at the same time; validation and resolving (groups and fqdns). It's the latter that makes it a bit complex, the validation "layer" is rather thin. Under these circumstances, I take any validation support I can get. In addition, your suggestion sounds kind of kludgy. Why would we limit our system and options with non-required constraints? Even if there where a guarantee that we always run servers with single, distinct purpose only (and there is none), why would we base our system's conditionals on something so debatable and thus volatile like a naming convention? On 2015/03/03 20:00:19, Wladimir Palant wrote: > Indeed, that's something we discussed previously - so I'm rather surprised to > see parsing of the local hosts.yaml here. The idea we discussed was rather > getting the list of hosts from the puppet master. So we would update the > repositories on the puppet master first, ask it for its hosts.yaml (something > along the lines of "ssh puppetmaster cat > /etc/puppet/infrastructure/hiera/private/hosts.yaml" should do) and do host > validation based on that list. I do remember that discussion, but IMHO the "idea we discussed" was just the base where we started. We also discussed the idea to use run.py and kick.py in all environments, not only production. This would be more clean, make the scripts useful for anybody (and actually testable!), but of course require a hosts.yaml to be the source of information. The plan here was to introduce another parameter, e.g. -m (for master) or -a (authority), so that fetching the info from another host is optional. This is still to be done, but for now this should be enough though - especially since we are trying to keep the changes at minimum. (One could even avoid having the private module cloned locally by just invoking both run.py and kick.py from the Puppet master in the meantime. This would require us to enable AgentForwarding, though.) http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.... run.py:67: item[1].get('dns', item[0]) for item in valid_hosts.items() On 2015/03/03 20:00:19, Wladimir Palant wrote: > for (key, value) in valid_hosts.iteritems() please - destructure the tuple > immediately, otherwise this is unreadable. > > Also, why tuple() if this really a list? > > chunk = [ > value.get('dns', key) for (key, value) in valid_hosts.iteritems() > ... > ] Done. To answer your question: There is no need for this to be a list, a tuple has all the features required here. I see that this may sound like over-thinking, but types are an aspect developers preferring statically typed languages always have in mind anyway, so this was more the result of an automatism. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.... run.py:72: or name in item[1]['ip'] On 2015/02/26 18:41:23, matze wrote: > While resolving IPs would be a nice feature, this line here is not enough. It > does not handle different phenotypes of the same IPv6 address, for example, and > also does not reflect that we actually allow a single IP to be given as a string > in `modules/base/manifests/init.pp`. Since the effort to integrate these facets > is way too high for now, I'll remove that line in the next patch-set (RC2). Done.
Still have to take a closer look, but here's some more comments for now. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/READ... File README.md (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/READ... README.md:61: ### Requirements On 2015/03/04 12:32:37, matze wrote: > On 2015/03/03 16:43:32, Felix H. Dahlke wrote: > > Shouldn't we also add hiera here? We should also describe the > > PUPPET_HIERA_CONFIG and PUPPET_HOSTS_CONFIG variables. > > Yes we should improve on documentation, yet this is not the right place do > document either Hiera or Puppet. These are the requirements you need to set up a > development environment. And in fact one does not need Puppet or Hiera for that > purpose - they are only used in the guest boxes. Oh, I actually presumed we need to have Hiera in the development environment - nevermind then. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/hier... File hiera/install-precise.py (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/hier... hiera/install-precise.py:86: add_key_process = subprocess.Popen(['apt-key', 'add', '-'], stdin=subprocess.PIPE) On 2015/03/04 12:32:37, matze wrote: > On 2015/03/03 16:43:32, Felix H. Dahlke wrote: > > All of this stuff looks like it'll work on any system using APT - not just > > Ubuntu, and not just precise either. Maybe we should call the script > > "install_apt.py" or something? > > If we do not actively develop for Debian, we cannot call it *-apt or *-debian. > Many derivatives maintain their own APT resources, with specific versions for > each package available. Ubuntu, in particular, tends to have more recent but > less stable versions. So just because it works on our boxes it does not mean it > works with APT-driven OS in general. IMHO we should be precise here (pun not > intended) and only indicate compatibility with the systems we've actually tested > that stuff on. > > However, just as an FYI, I can confirm that this actually works with Debian 7 - > the only other system I've tested so far, and that was just by accident. Fair enough. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.py File run.py (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.... run.py:56: host_names = set(str(item) for item in hosts) On 2015/03/04 12:32:37, matze wrote: > On 2015/03/03 20:00:19, Wladimir Palant wrote: > > Given that hosts is a list of string, this seems equivalent to |host_names = > > set(hosts)|. However, I don't see why we should convert it to a set at all. > > "Given that hosts is a list of string" <- this is an assumption; inside the > function there is no guarantee for that one. You know already that I don't like > dropping good practices without a good reason. > > But I do agree that there is no need for host_names to be a set, since > uniqueness is ensured in the result set already. That also means it can remain a > generator. Nit: The parentheses are now not necessary anymore. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.... run.py:60: valid_hosts = getValidHosts() On 2015/03/04 12:32:37, matze wrote: > On 2015/03/03 16:43:32, Felix H. Dahlke wrote: > > Host validation doesn't make _that_ much sense anymore now that the > development > > infrastructure doesn't have to mirror the production infrastructure anymore. > > That'd only work if we work with the real infrastructure-private rather than > > private-stub locally, which is probably not a good idea. > > > > How about relying on us sticking to our naming scheme consistently and just > > checking that there's a role for that server name? For example, we'd check > that > > there's a "downloadserver" role when the user wants to run something on a > server > > called "download\d". > > This version does two jobs at the same time; validation and resolving (groups > and fqdns). It's the latter that makes it a bit complex, the validation "layer" > is rather thin. Under these circumstances, I take any validation support I can > get. > > In addition, your suggestion sounds kind of kludgy. Why would we limit our > system and options with non-required constraints? Even if there where a > guarantee that we always run servers with single, distinct purpose only (and > there is none), why would we base our system's conditionals on something so > debatable and thus volatile like a naming convention? > > > On 2015/03/03 20:00:19, Wladimir Palant wrote: > > Indeed, that's something we discussed previously - so I'm rather surprised to > > see parsing of the local hosts.yaml here. The idea we discussed was rather > > getting the list of hosts from the puppet master. So we would update the > > repositories on the puppet master first, ask it for its hosts.yaml (something > > along the lines of "ssh puppetmaster cat > > /etc/puppet/infrastructure/hiera/private/hosts.yaml" should do) and do host > > validation based on that list. > > I do remember that discussion, but IMHO the "idea we discussed" was just the > base where we started. We also discussed the idea to use run.py and kick.py in > all environments, not only production. This would be more clean, make the > scripts useful for anybody (and actually testable!), but of course require a > hosts.yaml to be the source of information. The plan here was to introduce > another parameter, e.g. -m (for master) or -a (authority), so that fetching the > info from another host is optional. This is still to be done, but for now this > should be enough though - especially since we are trying to keep the changes at > minimum. > > (One could even avoid having the private module cloned locally by just invoking > both run.py and kick.py from the Puppet master in the meantime. This would > require us to enable AgentForwarding, though.) For the record: I actually prefer Wladimir's suggestion to mine. But to be practical: What IMO really gets in the way is any kind of validation that requires us to have the real private module checked out locally. Wladimir's suggestion sounds like something we want to do eventually, but if we want to keep things simple for now, I suggest we just get rid of the validation for now. http://codereview.adblockplus.org/4810150141493248/diff/5699127396532224/hier... File hiera/roles/monitoringserver.yaml (right): http://codereview.adblockplus.org/4810150141493248/diff/5699127396532224/hier... hiera/roles/monitoringserver.yaml:10: zone: "adblockplus.org" This was indented on column before. Certainly not a fan of that, but we should keep it consistent.
http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.py File run.py (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.... run.py:56: host_names = set(str(item) for item in hosts) On 2015/03/04 12:32:37, matze wrote: > "Given that hosts is a list of string" <- this is an assumption; inside the > function there is no guarantee for that one. You know already that I don't like > dropping good practices without a good reason. This isn't a library function you wrote here, it is a function used by this script alone. And the parameter passed in to the function is a list of strings, that's not an assumption. If you like you can document the parameter type. Note that you are now turning a list into a tuple, that makes no sense at all (see below on the difference between lists and tuples). http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.... run.py:60: valid_hosts = getValidHosts() On 2015/03/04 14:27:53, Felix H. Dahlke wrote: > But to be practical: What IMO really gets in the way is any kind of validation > that requires us to have the real private module checked out locally. Wladimir's > suggestion sounds like something we want to do eventually, but if we want to > keep things simple for now, I suggest we just get rid of the validation for now. Actually, changing the current validation code to download remote data should be a fairly trivial change. The only problem is, using and testing it can only be done once the puppet master has a hosts.yaml file checked out. So IMHO it is better to leave the validation as it is for now and then change it to use remote data as soon as the changes are pushed. While this change is implemented, not having a private/hosts.yaml file is enough to disable validation. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.... run.py:67: item[1].get('dns', item[0]) for item in valid_hosts.items() On 2015/03/04 12:32:37, matze wrote: > To answer your question: There is no need for this to be a list, a tuple has all > the features required here. The difference between a list and a tuple isn't the functionality they offer - they are semantically different. From the documentation: > Though tuples may seem similar to lists, they are often used in different situations and for different purposes. Tuples are immutable, and usually contain an heterogeneous sequence of elements that are accessed via unpacking (see later in this section) or indexing (or even by attribute in the case of namedtuples). Lists are mutable, and their elements are usually homogeneous and are accessed by iterating over the list.
No further comments from my end. On 2015/03/04 12:32:36, matze wrote: > Loosing overview is what happens when branches live too long.. ;) Long living branches is what happens when you try to stick everything into a single review instead of landing all the prerequisites first and making small steps ;)
Well then, let's wait for Felix'. I also still need feedback on my suggestion above.. :) http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.py File run.py (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.... run.py:56: host_names = set(str(item) for item in hosts) On 2015/03/04 15:14:11, Wladimir Palant wrote: > On 2015/03/04 12:32:37, matze wrote: > > "Given that hosts is a list of string" <- this is an assumption; inside the > > function there is no guarantee for that one. You know already that I don't > like > > dropping good practices without a good reason. > > This isn't a library function you wrote here, it is a function used by this > script alone. And the parameter passed in to the function is a list of strings, > that's not an assumption. If you like you can document the parameter type. > > Note that you are now turning a list into a tuple, that makes no sense at all > (see below on the difference between lists and tuples). Done. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.... run.py:56: host_names = set(str(item) for item in hosts) On 2015/03/04 14:27:53, Felix H. Dahlke wrote: > On 2015/03/04 12:32:37, matze wrote: > > On 2015/03/03 20:00:19, Wladimir Palant wrote: > > > Given that hosts is a list of string, this seems equivalent to |host_names = > > > set(hosts)|. However, I don't see why we should convert it to a set at all. > > > > "Given that hosts is a list of string" <- this is an assumption; inside the > > function there is no guarantee for that one. You know already that I don't > like > > dropping good practices without a good reason. > > > > But I do agree that there is no need for host_names to be a set, since > > uniqueness is ensured in the result set already. That also means it can remain > a > > generator. > > Nit: The parentheses are now not necessary anymore. Done. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.... run.py:60: valid_hosts = getValidHosts() On 2015/03/04 15:14:11, Wladimir Palant wrote: > On 2015/03/04 14:27:53, Felix H. Dahlke wrote: > > But to be practical: What IMO really gets in the way is any kind of validation > > that requires us to have the real private module checked out locally. > Wladimir's > > suggestion sounds like something we want to do eventually, but if we want to > > keep things simple for now, I suggest we just get rid of the validation for > now. @Felix As I wrote before, this is actually intended and just requires a later extension of run.py to integrate fetching from another resource. The code in this patch-set is already compatible with that, and the validation is just a minor part compared to the resolving. > Actually, changing the current validation code to download remote data should be > a fairly trivial change. The only problem is, using and testing it can only be > done once the puppet master has a hosts.yaml file checked out. So IMHO it is > better to leave the validation as it is for now and then change it to use remote > data as soon as the changes are pushed. While this change is implemented, not > having a private/hosts.yaml file is enough to disable validation. @Wladimir Yep, I think we're on the same page now. And yes the extension of run.py will follow as one of the first improvements after this merge - I do not intend on leaving that unfinished for too long. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.... run.py:67: item[1].get('dns', item[0]) for item in valid_hosts.items() On 2015/03/04 15:14:11, Wladimir Palant wrote: > On 2015/03/04 12:32:37, matze wrote: > > To answer your question: There is no need for this to be a list, a tuple has > all > > the features required here. > > The difference between a list and a tuple isn't the functionality they offer - > they are semantically different. From the documentation: > > > Though tuples may seem similar to lists, they are often used in different > situations and for different purposes. Tuples are immutable, and usually contain > an heterogeneous sequence of elements that are accessed via unpacking (see later > in this section) or indexing (or even by attribute in the case of namedtuples). > Lists are mutable, and their elements are usually homogeneous and are accessed > by iterating over the list. This has already been changed in the current patch-set, aligned with your previous comment. http://codereview.adblockplus.org/4810150141493248/diff/5699127396532224/hier... File hiera/roles/monitoringserver.yaml (right): http://codereview.adblockplus.org/4810150141493248/diff/5699127396532224/hier... hiera/roles/monitoringserver.yaml:10: zone: "adblockplus.org" On 2015/03/04 14:27:53, Felix H. Dahlke wrote: > This was indented on column before. Certainly not a fan of that, but we should > keep it consistent. Done.
Alright, finally took a thorough look! It's almost there, mostly got nits about stuff I didn't notice before. Also one general nit: Please ensure all files have exactly one newline at EOF. Some seem to have two how it looks here. http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.py File run.py (right): http://codereview.adblockplus.org/4810150141493248/diff/5647308616105984/run.... run.py:60: valid_hosts = getValidHosts() On 2015/03/04 15:14:11, Wladimir Palant wrote: > On 2015/03/04 14:27:53, Felix H. Dahlke wrote: > > But to be practical: What IMO really gets in the way is any kind of validation > > that requires us to have the real private module checked out locally. > Wladimir's > > suggestion sounds like something we want to do eventually, but if we want to > > keep things simple for now, I suggest we just get rid of the validation for > now. > > Actually, changing the current validation code to download remote data should be > a fairly trivial change. The only problem is, using and testing it can only be > done once the puppet master has a hosts.yaml file checked out. So IMHO it is > better to leave the validation as it is for now and then change it to use remote > data as soon as the changes are pushed. While this change is implemented, not > having a private/hosts.yaml file is enough to disable validation. Didn't realise this, but yeah, the catch clause still calls result.update. All good then, I can live with a warning. http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/READ... File README.md (right): http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/READ... README.md:22: defaults suitable for development and testing purpose. One can create a Should be: "testing purposeS" http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/READ... README.md:41: purpose are provided within `modules/private-stub/hiera`: "purposeS" again http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/READ... README.md:113: See `modules/base/manifests/init.pp`, especially the `explicit_host_record()` sounds weird, I'd say "especially the type named `explicit_host_record()`" http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/Vagr... File Vagrantfile (right): http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/Vagr... Vagrantfile:15: I'd rather not introduce extraneous whitespace here, also below. Let's keep unrelated changes low. http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/Vagr... Vagrantfile:28: setup = YAML.load_file(setup_path) rescue {} How I see it, if we get an exception here, setup is gonna be an empty hash. Won't the setup.fetch() call on the next line fail then? If so, I'd opt for leaving the rescue out here - then we get a meaningful exception at least. http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/Vagr... Vagrantfile:41: sudo /etc/puppet/infrastructure/hiera/install-precise.py Bit of a nit, but we're using underscores in Python scripts fairly consistently. Mandatory for modules (PEP8), but not strictly speaking for standalone scripts. Still, we don't have a single one using dashes, lets stay consistent. http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/Vagr... Vagrantfile:47: '--external_nodes=/etc/puppet/infrastructure/hiera/puppet-node-classifier.rb', While we don't have that many Ruby scripts so far, I'd opt for keeping the naming consistent with our Python scripts (i.e. underscores) - unless there's a reason against it. http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/Vagr... Vagrantfile:56: #puppet.hiera_config_path = 'hiera/vagrant.yaml' I'd rather remove this - not a fan of commented out code, and we'd also have to remove the --external_nodes line above anyway. I'd rather have this in an issue about switching to Puppet 3.x (which I really get the impression is something we want to do). http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/hier... File hiera/puppet-node-classifier.rb (right): http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/hier... hiera/puppet-node-classifier.rb:15: # The current script name, used for logging and usage hints That seems a bit too obvious for a comment IMO. Comments are useful when they add non-obvious information. Obvious comments that just repeat what's clearly in the code however are more likely to cause harm in my experience. For example, if (not when) they get out of sync with the code. http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/hier... hiera/puppet-node-classifier.rb:18: # Although there's no need for any options in particular yet, the script IMO also too obvious. It's a no-brainer that --help makes sense, and it's also a no-brainer to look up the docs for GetoptLong if one wants to add options. http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/hier... hiera/puppet-node-classifier.rb:51: # Only one additional non-option argument is allowed, in order to explicitly Also pretty obvious from the code IMO. http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/modu... File modules/base/manifests/init.pp (right): http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/modu... modules/base/manifests/init.pp:43: $servers = hiera("servers") Nit from the Puppet style guide: "All strings must be enclosed in single quotes, unless they contain variables or single quotes." https://docs.puppetlabs.com/guides/style_guide.html#quoting http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/modu... modules/base/manifests/init.pp:60: $fqdn_name = join([$name, $base::zone], ".") Nit: Should be single quotes here too. http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/modu... File modules/nagios/manifests/server.pp (right): http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/modu... modules/nagios/manifests/server.pp:179: $fqdn_name = join([$name, $nagios::server::zone], ".") Nit: Should be single quotes. http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/modu... modules/nagios/manifests/server.pp:182: use => 'generic-host', While the Puppet style guide actually wants us to align arrows on column, our style guide says something that overrules that: "Consistency is most important: Be consistent within functions, files, modules and projects. Making existing code conform with this style guide is fine, but it should happen in dedicated commits, preferably for a whole module or project at once." So we can either do this alignment in a dedicated commit, or overrule this part of the Puppet style guide in ours. http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/modu... File modules/statsmaster/manifests/init.pp (right): http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/modu... modules/statsmaster/manifests/init.pp:10: #realize known_hosts from base module Nit: Space after #.
http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/READ... File README.md (right): http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/READ... README.md:22: defaults suitable for development and testing purpose. One can create a On 2015/03/09 23:14:51, Felix H. Dahlke wrote: > Should be: "testing purposeS" Done. http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/READ... README.md:41: purpose are provided within `modules/private-stub/hiera`: On 2015/03/09 23:14:51, Felix H. Dahlke wrote: > "purposeS" again Done. http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/READ... README.md:113: See `modules/base/manifests/init.pp`, especially the `explicit_host_record()` On 2015/03/09 23:14:51, Felix H. Dahlke wrote: > sounds weird, I'd say "especially the type named `explicit_host_record()`" Fair enough, though it's actually a "named type" and not a "type named foo". Let's see if you like the new version more.. :) http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/Vagr... File Vagrantfile (right): http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/Vagr... Vagrantfile:28: setup = YAML.load_file(setup_path) rescue {} On 2015/03/09 23:14:51, Felix H. Dahlke wrote: > How I see it, if we get an exception here, setup is gonna be an empty hash. > Won't the setup.fetch() call on the next line fail then? If so, I'd opt for > leaving the rescue out here - then we get a meaningful exception at least. The next call works as well, because it also uses a default: Here setup.fetch(.., default) is pretty similar to Python's {}.get(.., default) or the .. rescue default before. http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/Vagr... Vagrantfile:41: sudo /etc/puppet/infrastructure/hiera/install-precise.py On 2015/03/09 23:14:51, Felix H. Dahlke wrote: > Bit of a nit, but we're using underscores in Python scripts fairly consistently. > Mandatory for modules (PEP8), but not strictly speaking for standalone scripts. > Still, we don't have a single one using dashes, lets stay consistent. Done. http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/Vagr... Vagrantfile:47: '--external_nodes=/etc/puppet/infrastructure/hiera/puppet-node-classifier.rb', On 2015/03/09 23:14:51, Felix H. Dahlke wrote: > While we don't have that many Ruby scripts so far, I'd opt for keeping the > naming consistent with our Python scripts (i.e. underscores) - unless there's a > reason against it. Done. http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/hier... File hiera/puppet-node-classifier.rb (right): http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/hier... hiera/puppet-node-classifier.rb:15: # The current script name, used for logging and usage hints On 2015/03/09 23:14:51, Felix H. Dahlke wrote: > That seems a bit too obvious for a comment IMO. Comments are useful when they > add non-obvious information. > > Obvious comments that just repeat what's clearly in the code however are more > likely to cause harm in my experience. For example, if (not when) they get out > of sync with the code. Done. http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/hier... hiera/puppet-node-classifier.rb:18: # Although there's no need for any options in particular yet, the script On 2015/03/09 23:14:51, Felix H. Dahlke wrote: > IMO also too obvious. It's a no-brainer that --help makes sense, and it's also a > no-brainer to look up the docs for GetoptLong if one wants to add options. Done. http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/hier... hiera/puppet-node-classifier.rb:51: # Only one additional non-option argument is allowed, in order to explicitly On 2015/03/09 23:14:51, Felix H. Dahlke wrote: > Also pretty obvious from the code IMO. Although I've improved some of the comments you've flagged here without further comment, let me point something out: I'm worrying that we do a pretty good job at criticizing and finding reasons to avoid comments, yet we do not really do a good job when it comes to documenting our code. "Too obvious" is a rather individual term ("obvious for whom?"), not a good set of criteria. Most of the time it results in "obvious for you" but a big pile of question marks for anyone who's not familiar with the entire system anyway. Like this very repository. The more objective practice is to avoid "repeating the code in the comments". And this snippet here is a very good example: It actually explains the intention behind the code and thus allows you to argue whether it does what it should do - or not. This is, IMHO, the bare minimum. http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/modu... File modules/base/manifests/init.pp (right): http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/modu... modules/base/manifests/init.pp:43: $servers = hiera("servers") On 2015/03/09 23:14:51, Felix H. Dahlke wrote: > Nit from the Puppet style guide: > > "All strings must be enclosed in single quotes, unless they contain variables or > single quotes." > > https://docs.puppetlabs.com/guides/style_guide.html#quoting Done. http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/modu... modules/base/manifests/init.pp:60: $fqdn_name = join([$name, $base::zone], ".") On 2015/03/09 23:14:51, Felix H. Dahlke wrote: > Nit: Should be single quotes here too. Done. http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/modu... File modules/nagios/manifests/server.pp (right): http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/modu... modules/nagios/manifests/server.pp:182: use => 'generic-host', On 2015/03/09 23:14:51, Felix H. Dahlke wrote: > While the Puppet style guide actually wants us to align arrows on column, our > style guide says something that overrules that: > > "Consistency is most important: Be consistent within functions, files, modules > and projects. Making existing code conform with this style guide is fine, but it > should happen in dedicated commits, preferably for a whole module or project at > once." > > So we can either do this alignment in a dedicated commit, or overrule this part > of the Puppet style guide in ours. Since arrow alignment is prone to indentation issues after e.g. merging, and because it naturally causes unrelated lines of code having to change sometimes, I deeply agree with you saying that we should come clear about this once and for all. So either we do not use arrow alignment (in which case I'll adjust all related files here quickly), or we use it. In the latter case, please leave the consistency part out of this particular patch set. I am actually planning on doing a style-update on most affected files directly after the push/merge, and even have a lot of pending patch sets already. The only reason why they're not in here is to minimize the changes.. http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/modu... File modules/statsmaster/manifests/init.pp (right): http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/modu... modules/statsmaster/manifests/init.pp:10: #realize known_hosts from base module On 2015/03/09 23:14:51, Felix H. Dahlke wrote: > Nit: Space after #. Done.
LGTM, assuming you answer "no" to my question. http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/Vagr... File Vagrantfile (right): http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/Vagr... Vagrantfile:28: setup = YAML.load_file(setup_path) rescue {} On 2015/03/16 12:07:11, matze wrote: > On 2015/03/09 23:14:51, Felix H. Dahlke wrote: > > How I see it, if we get an exception here, setup is gonna be an empty hash. > > Won't the setup.fetch() call on the next line fail then? If so, I'd opt for > > leaving the rescue out here - then we get a meaningful exception at least. > > The next call works as well, because it also uses a default: Here > setup.fetch(.., default) is pretty similar to Python's {}.get(.., default) or > the .. rescue default before. But setup would be nill - can we call "fetch" on nil without exceptions?
http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/Vagr... File Vagrantfile (right): http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/Vagr... Vagrantfile:28: setup = YAML.load_file(setup_path) rescue {} On 2015/03/16 18:10:15, Felix H. Dahlke wrote: > On 2015/03/16 12:07:11, matze wrote: > > On 2015/03/09 23:14:51, Felix H. Dahlke wrote: > > > How I see it, if we get an exception here, setup is gonna be an empty hash. > > > Won't the setup.fetch() call on the next line fail then? If so, I'd opt for > > > leaving the rescue out here - then we get a meaningful exception at least. > > > > The next call works as well, because it also uses a default: Here > > setup.fetch(.., default) is pretty similar to Python's {}.get(.., default) or > > the .. rescue default before. > > But setup would be nill - can we call "fetch" on nil without exceptions? It won't be nil. It is either the parsed YAML or an empty hash after the load_file/rescue operation, and the subsequent fetch() also results in either a YAML object or another, empty hash. By the way, the load_file() operation failing happens all the time. We only have 3 roles integrated yet, and all the others trigger an exception here. And the script does indeed continue as expected in these cases. Furthermore, only the infraforumserver.yaml file features a "requirements" section so far - yet monitoringserver.yaml and codereviewserver.yaml work perfectly fine. However, I do agree that a future version should be more verbose here. E.g. when the #{role}.yaml exists, but parsing fails, actually print an error message and probably even exit..
http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/Vagr... File Vagrantfile (right): http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/Vagr... Vagrantfile:28: setup = YAML.load_file(setup_path) rescue {} On 2015/03/16 18:10:15, Felix H. Dahlke wrote: > But setup would be nill - can we call "fetch" on nil without exceptions? As you said yourself, setup would be {} in case of an exception rather than nil - and {}.fetch("requirements", {}) will give you {}, all fine.
LGTM http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/Vagr... File Vagrantfile (right): http://codereview.adblockplus.org/4810150141493248/diff/5720605454237696/Vagr... Vagrantfile:28: setup = YAML.load_file(setup_path) rescue {} On 2015/03/16 19:09:42, Wladimir Palant wrote: > On 2015/03/16 18:10:15, Felix H. Dahlke wrote: > > But setup would be nill - can we call "fetch" on nil without exceptions? > > As you said yourself, setup would be {} in case of an exception rather than nil > - and {}.fetch("requirements", {}) will give you {}, all fine. Right, missed that yesterday, works fine indeed. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
