[node-patches] Change in ovirt-node[puppet-ovirt-node-plugin]: Initial push of puppet plugin code to ovirt repo
fabiand at fedoraproject.org
fabiand at fedoraproject.org
Tue May 14 11:02:27 UTC 2013
Fabian Deutsch has posted comments on this change.
Change subject: Initial push of puppet plugin code to ovirt repo
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(25 inline comments)
Awesome to see this patch land!
In all it looks good - but I must admit that I'm not familiar with ruby ... So I'll take another close look the next time.
Gerrit doesn't seem to do reviews on files with leading dots, so please drop: plugins/puppet/provider/type/.ovirt_node.rb.swp
....................................................
File plugins/puppet/autogen.sh
Line 1: #!/bin/sh
Do we still need this file?
Line 2: # Run this to generate configure and Makefile
Line 3:
Line 4: srcdir=`dirname $0`
Line 5: test -z "$srcdir" && srcdir=.
....................................................
File plugins/puppet/build/COPYING
Line 1: GNU GENERAL PUBLIC LICENSE
This file and the whole build/ dir can be dropped and should be added to .gitignore
Line 2: Version 2, June 1991
Line 3:
Line 4: Copyright (C) 1989, 1991 Free Software Foundation, Inc.
Line 5: 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
....................................................
File plugins/puppet/configure.ac
Line 1: AC_INIT([puppet-ovirt-node-plugin], [0.0.1], [node-devel at ovirt.org])
I suppos enot needed anymore as it's now part of ovirt-node
Line 2: AM_INIT_AUTOMAKE([-Wall -Werror foreign -Wno-portability tar-pax])
Line 3:
Line 4: AM_PATH_PYTHON
Line 5:
....................................................
File plugins/puppet/Makefile.am
Line 28: recipe
Line 29:
Line 30: EXTRA_DIST = \
Line 31: puppet-ovirt-node-plugin.spec \
Line 32: puppet-ovirt-node-plugin.spec.in
A trailing whitespace
Line 33:
Line 34: DISTCLEANFILES = $(PACKAGE)-$(VERSION).tar.gz \
Line 35: aclocal.m4 \
Line 36: configure \
Line 39: py-compile
Line 40:
Line 41: DISTCLEANDIRS = autom4te.cache
Line 42:
Line 43: rpms: dist
I suppose you can remove everything else below this line.
rpms are not needed - or it is specified as a subpackage in ovirt-node.spec.in - and checks are in the check-makefile - maybe the plugin path should be added to the PYTHONSOURCES in the src/Makefile.check
Line 44: rpmbuild $(RPM_FLAGS) -ta $(distdir).tar.gz
Line 45:
Line 46: srpms: dist
Line 47: rpmbuild $(RPM_FLAGS) -ts $(distdir).tar.gz
....................................................
File plugins/puppet/provider/lib/ovirt_node.rb
Line 14: python("-c 'from ovirt.node.config import defaults; model = defaults.#{type}(); model.update(#{value}); tx = model.transaction(); tx()'")
Line 15: end
Line 16:
Line 17: def instances
Line 18: config_vars = %w[hostname ssh_pwauth dns ip_address ip_gateway ip_netmask
trailing whitespace
Line 19: management_server use_strong_rng]
Line 20: if FileTests.exists?("/etc/pki/vdsm/certs/engine_web_ca.pem")
Line 21: engine = %x(openssl x509 -in /etc/pki/vdsm/certs/engine_web_ca.pem -noout -issuer).
Line 22: match(/CN=CA-(.*?)\.\d+$/)
Line 46: puts "Do nothing"
Line 47: #Waiting to see logic from new VDSM plugin
Line 48: end
Line 49:
Line 50: def nfsdomain=(domain)
trailing whitespace
Line 51: # The py snippet below will only change the runtime configuratio (NFSv4 domain)
Line 52: # To make persistent changes we'll need to use the ovirt.node.config.defaults
Line 53: # module, which gives us access to all topics which are covered by Node's logic.
Line 54: # ovirt.node.utils.* modules only change the runtime config, but don't persist the
Line 56: # See ssh_pwauth for a complete example
Line 57: update_wrapper("NFS", domain)
Line 58: end
Line 59:
Line 60: def iscsi_initiator=(name)
trailing white-space
Line 61: update_wrapper("iSCSI", name)
Line 62: end
Line 63:
Line 64: def ssh=(bool)
Line 60: def iscsi_initiator=(name)
Line 61: update_wrapper("iSCSI", name)
Line 62: end
Line 63:
Line 64: def ssh=(bool)
trailing white-space
Line 65: # It needs to be ensured that #{bool} is either True or False (so a bool in py syntax)
Line 66: value = ""
Line 67: if bool
Line 68: value = "True"
Line 85: end
Line 86:
Line 87: def rsyslog=(server)
Line 88: server, port = server.split(":")[0]
Line 89: port = port ? port : 7634
is this always the same port?
Line 90: update_wrapper("Syslog", "#{server}, #{port}")
Line 91: end
Line 92:
Line 93: def netconsole=(server)
Line 91: end
Line 92:
Line 93: def netconsole=(server)
Line 94: server, port = server.split(":")[0]
Line 95: port = port ? port : 7634
is this always the same port?
Line 96: update_wrapper("Netconsole", "#{server}, #{port}")
Line 97: end
Line 98:
Line 99: def monitoring=(server)
Line 97: end
Line 98:
Line 99: def monitoring=(server)
Line 100: server, port = server.split(":")[0]
Line 101: port = port ? port : 7634
is this always the same port?
Line 102: update_wrapper("Collectd", "#{server}, #{port}")
Line 103: end
Line 104:
Line 105: def ntp=(servers)
Line 104:
Line 105: def ntp=(servers)
Line 106: update_wrapper("Timeservers", "[#{servers.join(", ")}]")
Line 107: end
Line 108:
trailing white-spaces
Line 109: def ntp=(servers)
Line 110: update_wrapper("Nameservers", "[#{servers.join(", ")}]")
Line 111: end
Line 112:
....................................................
File plugins/puppet/provider/type/ovirt_node.rb
Line 1: Puppet::Type.newtype(:ovirt_node) do
A license header?
Line 2: ensurable
Line 3:
Line 4: newparam(:address, :namevar => true) do
Line 5: desc "The address of the server"
Line 1: Puppet::Type.newtype(:ovirt_node) do
Line 2: ensurable
Line 3:
Line 4: newparam(:address, :namevar => true) do
Line 5: desc "The address of the server"
of which server?
Engine? Just to make it clear
Line 6: end
Line 7:
Line 8: newparam(:nfsdomain) do
Line 9: desc "The NFSv4 domain for the node"
Line 15:
Line 16: newparam(:kdump) do
Line 17: desc "kdump configuration"
Line 18: end
Line 19:
trailing whitespaces.
Line 20: newparam(:ssh) do
Line 21: desc "Enable SSH authentication"
Line 22: newvalues(:True, :true, :False, :false)
Line 23: defaultto :False
Line 33: end
Line 34: end
Line 35: end
Line 36:
Line 37: newparam(:rsyslog) do
trailing white-space
Line 38: desc "RSyslog server"
Line 39: end
Line 40:
Line 41: newparam(:netconsole) do
....................................................
File plugins/puppet/puppet-ovirt-node-plugin.spec.in
Line 1: Summary: A plugin to make oVirt Node installs compatible with Puppet
Can we drop this file - when the plugin is properly merged into the top-level ovirt-node.spec.in file?
Line 2: Name: puppet-ovirt-node-plugin
Line 3: Version: @VERSION@
Line 4: Release: 0%{?BUILD_NUMBER}%{?extra_release}%{?dist}
Line 5: Source0: %{name}-%{version}.tar.gz
....................................................
File plugins/puppet/README.md
Line 1: puppet-ovirt-node-plugin
Line 2: ========================
Line 3:
Line 4: ./autogen.sh && make rpms
not correct anymore, it's now a subpackage of ovirt-node, isn't it?
....................................................
File plugins/puppet/src/Makefile.am
Line 30:
Line 31: rbovirtsetupdir = $(localstatedir)/lib/puppet/facts
Line 32: rbovirtsetup_SCRIPTS = \
Line 33: ovirt.rb
Line 34:
can we remove the checks and include the src/Makefile.check file instead?
Line 35: # Requires python-pep8 package (Fedora)
Line 36: check-local: check-local-pep8
Line 37:
Line 38: check-local-pep8:
....................................................
File plugins/puppet/src/ovirt.rb
Line 1: Facter.add(:operatingsystem) do
A license header?
Line 2: has_weight 100_000_000
Line 3: confine :kernel => :linux
Line 4: setcode do
Line 5: if FileTest.exists?("/etc/default/version")
Line 1: Facter.add(:operatingsystem) do
Line 2: has_weight 100_000_000
Line 3: confine :kernel => :linux
Line 4: setcode do
Line 5: if FileTest.exists?("/etc/default/version")
Basically it's fine to use default/version - but I'd rather like to use
cat /etc/system-release-cpe /etc/system-release /etc/os-release
They are a standard and defaults/ is deprecated.
We just need to make sure that RHEV and Node are using/updating these files.
Line 6: txt = File.read("/etc/default/version")
Line 7: if txt =~ /^PRODUCT='(.*?)\s/
Line 8: $1
Line 9: end
Line 13:
Line 14: Facter.add(:operatingsystemrelease) do
Line 15: confine :operatingsystem => %w{oVirt}
Line 16: setcode do
Line 17: if FileTest.exists?("/etc/default/version")
same as above
Line 18: txt = File.read("/etc/default/version")
Line 19: if txt =~ /^VERSION=(.*)/
Line 20: $1
Line 21: else
....................................................
File plugins/puppet/src/puppet_page.py
Line 145: for line in lines:
Line 146: try:
Line 147: item = re.match(r'^\s+(\w+) =', line).group(1)
Line 148: if item in cfg:
Line 149: print "matched"
self.logger is the place to dump dirt :)
Line 150: conf.write(re.sub(r'(^.*?' + item + ' =).*', r'\1 "' +
Line 151: cfg[item] + '"', line))
Line 152: except:
Line 153: conf.write(line)
Line 151: cfg[item] + '"', line))
Line 152: except:
Line 153: conf.write(line)
Line 154:
Line 155: utils.process.call("service puppet stop")
shouldn't this be a restart or start-stop cycle?
Line 156: #utils.process.check_call("puppet agent --test")
--
To view, visit http://gerrit.ovirt.org/14664
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb639acf019a48e809fb983ab2124c6215c00157
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-node
Gerrit-Branch: puppet-ovirt-node-plugin
Gerrit-Owner: Ryan Barry <rbarry at redhat.com>
Gerrit-Reviewer: Fabian Deutsch <fabiand at fedoraproject.org>
More information about the node-patches
mailing list