summaryrefslogtreecommitdiffstats
path: root/lib/puppet/util
Commit message (Collapse)AuthorAgeFilesLines
...
| * | Kludge for #5206 -- port of fix for #3536 to yamlMarkus Roberts2010-11-101-1/+4
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The internal format of parameters was changed between 0.25.x and 2.6.x, making them incompatible when serialized. This was fixed for PSON under ticket #3656 (by modifying the data as it was serialized) in: commit f66095d35bc5f9645eb19bbb8cefa342c0181d2d and commit 2edf7fe968ac3d8af9865f65100940747c1fa894 but nothing was done about the other serialization formats. This commit adds a callback to zaml property serialization and uses it to route data through the fixup method added in the commits above, thus fixing the problem "the same way" as it was fixed for PSON under #3656. It does nothing about marshal, mostly because I have not totaly gone over to the dark side (yet).
* | | Merge branch 'ticket/next/4590' into nextMarkus Roberts2010-11-171-0/+15
|\ \ \ | |_|/ |/| | | | | | | | | | | Conflicts: lib/puppet/util/monkey_patches.rb -- two unrelated additions had been made, kept them both.
| * | [#4590] SimpleGraph is slowPaul Berry2010-10-071-0/+13
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Rewrote SimpleGraph to use a more efficient internal representation. To preserve compatibility with older clients, graphs are still serialized to YAML using the format used by Puppet 2.6. However, a newer, more compact format can be enabled by setting "use_new_yaml_format" to true. Deserialization from YAML accepts either the old 2.6 format or the newer format. In a later release, once we no longer need to be compatible with 2.6, we will be able to switch to the new format. To make deserialization accept multiple formats, it was necessary to use the yaml_initialize method. This method is not supported in versions of Ruby prior to 1.8.3, so a monkey patch is included to add support for it to Ruby 1.8.1 and 1.8.2. Thanks to Markus Roberts for the SimpleGraph rewrite. Thanks to Jesse Wolfe for figuring out how to write the yaml_initialize monkey patch.
* | | Merge commit '2.6.3rc2' into nextPaul Berry2010-11-091-2/+0
|\ \ \ | | |/ | |/| | | | | | | | | | Manually resolved conflicts: lib/puppet/parser/ast/resource.rb spec/unit/parser/ast/resource_spec.rb
| * | Fixed #5137 - Removed no longer required TOC referencesJames Turnbull2010-10-291-2/+0
| | |
* | | maint: prevent fork bombs by disabling ActiveSupport's Kernel.daemonizeJesse Wolfe2010-11-091-0/+8
| | | | | | | | | | | | | | | | | | | | | | | | | | | ActiveSupport provides a "daemonize" method on all objects that causes the ruby process to fork to the background. This is extremely surprising and dangerous, and some of our spec tests could trigger this accidentally. This patch adds a "daemonize" method to Object which shadows the ActiveSupport version, preventing it from ever being called.
* | | Merge commit '2.6.2' into nextJesse Wolfe2010-10-121-1/+1
|\| |
| * | Fixed #4919 - added parenths to fix error message:James Turnbull2010-10-021-1/+1
| | | | | | | | | | | | /usr/lib/ruby/site_ruby/1.8/puppet/util/metric.rb:62: warning: parenthesize argument(s) for future version
* | | Merge commit '2.6.2rc1' into nextJesse Wolfe2010-10-041-1/+0
|\| |
| * | Fix for #4896 -- stray newline left over from removed diagnosticMarkus Roberts2010-09-301-1/+0
| | | | | | | | | | | | | | | A newline that was part of a diagnostic was left in, and this caused problems with the serialization of strings in "preserve newlines" mode.
* | | Partial merge to 2.6.2rc1 : Merge commit '574812e' into nextJesse Wolfe2010-10-041-10/+12
|\| | | | | | | | | | | | | | | | | | | | | | | | | | The API for AST objects has changed, making the initialization of this spec somewhat more complicated. Also, git seems to have been confused by whitespace changes. Manually Resolved Conflicts: lib/puppet/parser/ast/resource.rb
| * | (#4860) Fix wrong method name.. params seems to be renamed to parametersSimon Effenberg2010-09-281-1/+1
| | |
* | | Partial merge to 2.6.2rc1 : Merge commits 'd057b90' and '6dd1930' into nextJesse Wolfe2010-10-041-9/+38
|\| | | | | | | | | | | | | | This commit merges in d057b90 with the addition of commit 6dd1930 to fix some tests that are confined to machines where rrd is installed.
| * | Fix #4726 Update puppet rrdtool metric code to support modern rrd ruby bindingsJesse Wolfe2010-09-281-9/+38
| | | | | | | | | | | | | | | | | | | | | | | | | | | The rrd project has been shipping ruby bindings since 1.3.0, and the old rrdtool ruby library is no longer being maintained. This patch is based upon Davor Ocelic's submitted code, with the addition that I've added conditionals so it can still call the old rrdtool library if a modern rrd is not installed.
* | | Partial merge to 2.6.2rc1 : Merge commit '99c1019' into nextJesse Wolfe2010-10-041-9/+11
|\| | | | | | | | | | | | | | | | | | | | | | | | | | | | | This conflict was a little too complicated for diff to figure out, but the resolution is actually fairly mechanical. Some unit tests changed because they were mocking unnecessarily. Manually Resolved Conflicts: lib/puppet/util/rdoc/parser.rb spec/unit/util/rdoc/parser_spec.rb
| * | [#4798] Puppet doc manifests documentation mode brokenPaul Berry2010-09-221-6/+9
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | When running puppet doc, if the directory containing the user's specified manifest file overlaps with the modules directory (i.e. they are the same directory or one contains the other), Puppet doc would try to parse the overlapping files twice, triggering an exception which made the documentation run fail. Fixed the bug by adding a check to the RDoc::Parser#scan method to prevent re-parsing of files that have already been parsed. Also added a spec test to verify that this works.
* | | Partial merge to 2.6.2rc1 : Merge commit '7b8cb74' into nextJesse Wolfe2010-10-021-4/+4
|\| | | | | | | | | | | There are merge conflicts with commits following this one.
| * | Fixed Puppet Doc TOC generationJames Turnbull2010-09-161-2/+2
| | |
| * | Documentation updates for Markdown conversionJames Turnbull2010-09-141-2/+2
| | |
* | | Merge remote branch 'paul/ticket/next/4570' into nextJesse Wolfe2010-09-081-12/+11
|\ \ \ | |_|/ |/| |
| * | Fixed issue #4570 (Race conditions when serializing objects to YAML).Paul Berry2010-08-231-12/+11
| |/ | | | | | | | | | | | | | | | | | | | | | | | | The ZAML class was using class variables to keep track of labels and backreferences while serializing object to YAML. This made it possible to get ill-formed or incorrect YAML output if two threads tried to serialize objects at the same time. Changed to use instance variables of the ZAML class, so there is no race condition. Also added some more spec tests to verify that labels are generated properly.
* | Merge remote branch 'paul/4472-4483-4496-4521-4522'Jesse Wolfe2010-08-171-5/+9
|\ \ | |/ |/| | | a.k.a. "make_taller_trees"
| * [#4496]+[#4521]+[#4522] Add structures to the AST to represent type ↵Paul Berry2010-08-131-5/+9
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | definitions (classes, definitions, and nodes). Previously, type definitions were not represented directly in the AST. Instead, the parser would instantiate types and insert them into known_resource_types as soon as they were parsed. This made it difficult to distinguish which types had come from the file that was just parsed and which types had been loaded previously, which led to bug 4496. A side-effect of this change is that the user is no longer allowed to define types inside of conditional constructs (such as if/else). This was allowed before but had unexpected semantics (bugs 4521 and 4522). It is still possible, however, to place an "include" statement inside a conditional construct, and have that "include" statement trigger the autoloading of a file that instantiates types.
* | Further RST to Markdown fixes for types, values, testsJames Turnbull2010-08-124-46/+19
| |
* | Updated all types to Markdown outputJames Turnbull2010-08-111-5/+4
| | | | | | | | | | | | | | | | | | | | Updated defaults.rb to Markdown Migrated report documentation to Markdown Migrated Naginator doco to Markdown Updated provider documentation to Markdown
* | Updated reference output to generate valid MarkdownJames Turnbull2010-08-113-15/+9
| |
* | Removed legacy Trac codeJames Turnbull2010-08-111-24/+1
| |
* | Fix for #4476 -- rails calling yaml internalsMarkus Roberts2010-08-061-1/+1
| | | | | | | | | | | | | | | | Some recent versions of rails have started calling yaml with the internal (and IIRC depricated) signature; this does nothing special in yaml but it breaks with zaml, so we add an ignored parameter to simulate the non-behavior. *sigh* I love rails.
* | Fix #4461 - attempt to fix another performance issueBrice Figureau2010-08-031-5/+17
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | During a profiling of a 2.6.1rc1 puppet master, I found that we spend a lot of time and efforts in Puppet::Util::Autoload#module_directories. Since this method is doing a bunch of filesystem access, this process is slow. In fact each time we were evaluating a resource or trying to find if a given resource was a builtin type we ended up scanning the whole module directories for the given environment. This patch attempts to fix this performance issue by caching the module_directories output for the either the time of the compilation or an agent configurer run (since this stuff looks like to be shared for both compilation and catalog evaluation). With this patch, my compilation time for 2k resources went from 5.91s to 3.71s (second run each time to allievate parsing time).. Signed-off-by: Brice Figureau <brice-puppet@daysofwonder.com> Edited to fix a typo [#4434] Signed-off-by: Jesse Wolfe <jes5199@gmail.com>
* | Removed eventual documentation line ... eventually cameJames Turnbull2010-07-301-2/+0
|/
* Fix #4244 - Cached Attributes is not thread safeBrice Figureau2010-07-251-10/+15
| | | | | | | | | | The underlying hash is not protected and thus two threads accessing the cached value at the same time and one expiring the value can result in a race condition. This patch synchronizes the access to the value_cache underlying hash. Signed-off-by: Brice Figureau <brice-puppet@daysofwonder.com>
* JRuby doesn't implement Process.maxgroupsBrice Figureau2010-07-251-1/+4
| | | | | | So let's not call it :) Signed-off-by: Brice Figureau <brice-puppet@daysofwonder.com>
* Fix #4348 - Puppet doc single manifest brokenBrice Figureau2010-07-251-1/+1
| | | | | | | | The refactoring of using environment instances instead of strings for initializing the parser, rdoc wasn't updated, thus was unable to initialize the parser. Signed-off-by: Brice Figureau <brice-puppet@daysofwonder.com>
* Fix #4286 - rename puppetdoc global module <site> to __site__Brice Figureau2010-07-251-3/+5
| | | | | | | | | < and > might be invalid or borderline chars to use for a file name or an url. This patch changes those characters to __. Signed-off-by: Brice Figureau <brice-puppet@daysofwonder.com>
* Fixing #4268 - manifests always importedLuke Kanies2010-07-191-15/+6
| | | | | | | | | | | | | | The problem is that the environment list gets cleared when Settings#set_value is called, and it was being called every time Settings#use was called, which is more often than obvious but especially if reporting is enabled. Previously we ignored noop when running inside of Settings, and this essentially adds that back in, so we can remove the special noop behaviour in Settings itself. Signed-off-by: Luke Kanies <luke@puppetlabs.com>
* Fixes errant Trac references in documentationJames Turnbull2010-07-156-6/+6
|
* [#4215] Have rundir depend on vardirMatt Robinson2010-07-131-9/+9
| | | | | | | | | | | | This came up because if you ran puppetd with a specific vardir, then when rundir got set to a hardcoded value its parent directory might not exist and the whole thing would fail. This change came about with the concept of run_mode, and this fix is restoring the behaviour that was in 0.25.x Reviewed-by: Jesse Wolfe Signed-off-by: Matt Robinson <matt@puppetlabs.com>
* Code smell: Two space indentationMarkus Roberts2010-07-0963-6916/+6916
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Replaced 106806 occurances of ^( +)(.*$) with The ruby community almost universally (i.e. everyone but Luke, Markus, and the other eleven people who learned ruby in the 1900s) uses two-space indentation. 3 Examples: The code: end # Tell getopt which arguments are valid def test_get_getopt_args element = Setting.new :name => "foo", :desc => "anything", :settings => Puppet::Util::Settings.new assert_equal([["--foo", GetoptLong::REQUIRED_ARGUMENT]], element.getopt_args, "Did not produce appropriate getopt args") becomes: end # Tell getopt which arguments are valid def test_get_getopt_args element = Setting.new :name => "foo", :desc => "anything", :settings => Puppet::Util::Settings.new assert_equal([["--foo", GetoptLong::REQUIRED_ARGUMENT]], element.getopt_args, "Did not produce appropriate getopt args") The code: assert_equal(str, val) assert_instance_of(Float, result) end # Now test it with a passed object becomes: assert_equal(str, val) assert_instance_of(Float, result) end # Now test it with a passed object The code: end assert_nothing_raised do klass[:Yay] = "boo" klass["Cool"] = :yayness end becomes: end assert_nothing_raised do klass[:Yay] = "boo" klass["Cool"] = :yayness end
* Code smell: Avoid needless decorationsMarkus Roberts2010-07-0915-30/+30
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | * Replaced 704 occurances of (.*)\b([a-z_]+)\(\) with \1\2 3 Examples: The code: ctx = OpenSSL::SSL::SSLContext.new() becomes: ctx = OpenSSL::SSL::SSLContext.new The code: skip() becomes: skip The code: path = tempfile() becomes: path = tempfile * Replaced 31 occurances of ^( *)end *#.* with \1end 3 Examples: The code: becomes: The code: end # Dir.foreach becomes: end The code: end # def becomes: end
* Code smell: Don't restate results directly after assignmentMarkus Roberts2010-07-096-7/+0
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Replaced 33 occurances of ([$@]?\w+)( +[|&+-]{0,2}= .+) \1 end with 3 Examples: The code: @sync ||= Sync.new @sync end becomes: @sync ||= Sync.new end The code: str += "\n" str end becomes: str += "\n" end The code: @indirection = Puppet::Indirector::Indirection.new(self, indirection, options) @indirection end becomes: @indirection = Puppet::Indirector::Indirection.new(self, indirection, options) end
* Code smell: Use &&= for dependent initializationMarkus Roberts2010-07-092-4/+4
| | | | | | | | | | | | | | | | | | | | | | | | | Replaced 6 occurances of ([$@]?\w+) += +(.*) +(if +\1|unless +\1.nil\?)$ with \1 &&= \2 3 Examples: The code: end becomes: end The code: becomes: The code: res becomes: res
* Code smell: Use ||= for conditional initializationMarkus Roberts2010-07-098-9/+9
| | | | | | | | | | | | | | | | | | | | | | | | | Replaced 55 occurances of ([$@]?\w+) += +(.*) +(if +\1.nil\?|if +! *\1|unless +\1|unless +defined\?\(\1\))$ with \1 ||= \2 3 Examples: The code: @sync becomes: @sync The code: becomes: The code: if @yydebug becomes: if @yydebug
* Code smell: Omit needless checks on definedMarkus Roberts2010-07-098-12/+12
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | * Replaced 53 occurances of defined\?\((.+?)\) (?:and|&&) \1( |$) with \1\2 In code like: unless defined? @foo and @foo and bar("baz") "defined? @foo and @foo" can safely be replaced with "@foo": unless @foo and bar("baz") Because: * Both evaluate to false/nil when @foo is not defined * Both evaluate to @foo when @foo is defined 3 Examples: The code: @sync = Sync.new unless defined?(@sync) and @sync becomes: @sync = Sync.new unless @sync The code: unless defined?(@content) and @content becomes: unless @content The code: raise(ArgumentError, "Already handling indirection for #{@indirection.name}; cannot also handle #{indirection}") if defined?(@indirection) and @indirection becomes: raise(ArgumentError, "Already handling indirection for #{@indirection.name}; cannot also handle #{indirection}") if @indirection * Replaced 2 occurances of defined\?\((.+?)\) (?:and|&&) ! *\1.nil\? with !\1.nil? In code like: while defined? @foo and ! @foo.nil? ... "defined? @foo and ! @foo.nil?" can safely be replaced with "! @foo.nil?": while ! @foo.nil? ... Because: * Both evaluate to false/nil when @foo is not defined * Both evaluate to "! @foo.nil?" when @foo is defined 2 Examples: The code: !!(defined?(@value) and ! @value.nil?) becomes: !!(!@value.nil?) The code: self.init unless defined?(@@state) and ! @@state.nil? becomes: self.init unless !@@state.nil?
* Code smell: Avoid unneeded blocksMarkus Roberts2010-07-095-27/+9
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Replaced 45 occurances of (DEF) begin (LINES) rescue(.*) (LINES) end end with 3 Examples: The code: def find(name) begin self.const_get(name.to_s.capitalize) rescue puts "Unable to find application '#{name.to_s}'." Kernel::exit(1) end end becomes: def find(name) self.const_get(name.to_s.capitalize) rescue puts "Unable to find application '#{name.to_s}'." Kernel::exit(1) end The code: def exit_on_fail(message, code = 1) begin yield rescue RuntimeError, NotImplementedError => detail puts detail.backtrace if Puppet[:trace] $stderr.puts "Could not #{message}: #{detail}" exit(code) end end becomes: def exit_on_fail(message, code = 1) yield rescue RuntimeError, NotImplementedError => detail puts detail.backtrace if Puppet[:trace] $stderr.puts "Could not #{message}: #{detail}" exit(code) end The code: def start begin case ssl when :tls @connection = LDAP::SSLConn.new(host, port, true) when true @connection = LDAP::SSLConn.new(host, port) else @connection = LDAP::Conn.new(host, port) end @connection.set_option(LDAP::LDAP_OPT_PROTOCOL_VERSION, 3) @connection.set_option(LDAP::LDAP_OPT_REFERRALS, LDAP::LDAP_OPT_ON) @connection.simple_bind(user, password) rescue => detail raise Puppet::Error, "Could not connect to LDAP: #{detail}" end end becomes: def start case ssl when :tls @connection = LDAP::SSLConn.new(host, port, true) when true @connection = LDAP::SSLConn.new(host, port) else @connection = LDAP::Conn.new(host, port) end @connection.set_option(LDAP::LDAP_OPT_PROTOCOL_VERSION, 3) @connection.set_option(LDAP::LDAP_OPT_REFERRALS, LDAP::LDAP_OPT_ON) @connection.simple_bind(user, password) rescue => detail raise Puppet::Error, "Could not connect to LDAP: #{detail}" end
* Code smell: Avoid explicit returnsMarkus Roberts2010-07-0930-89/+89
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Replaced 583 occurances of (DEF) (LINES) return (.*) end with 3 Examples: The code: def consolidate_failures(failed) filters = Hash.new { |h,k| h[k] = [] } failed.each do |spec, failed_trace| if f = test_files_for(failed).find { |f| failed_trace =~ Regexp.new(f) } filters[f] << spec break end end return filters end becomes: def consolidate_failures(failed) filters = Hash.new { |h,k| h[k] = [] } failed.each do |spec, failed_trace| if f = test_files_for(failed).find { |f| failed_trace =~ Regexp.new(f) } filters[f] << spec break end end filters end The code: def retrieve return_value = super return_value = return_value[0] if return_value && return_value.is_a?(Array) return return_value end becomes: def retrieve return_value = super return_value = return_value[0] if return_value && return_value.is_a?(Array) return_value end The code: def fake_fstab os = Facter['operatingsystem'] if os == "Solaris" name = "solaris.fstab" elsif os == "FreeBSD" name = "freebsd.fstab" else # Catchall for other fstabs name = "linux.fstab" end oldpath = @provider_class.default_target return fakefile(File::join("data/types/mount", name)) end becomes: def fake_fstab os = Facter['operatingsystem'] if os == "Solaris" name = "solaris.fstab" elsif os == "FreeBSD" name = "freebsd.fstab" else # Catchall for other fstabs name = "linux.fstab" end oldpath = @provider_class.default_target fakefile(File::join("data/types/mount", name)) end
* Code smell: Booleans are first class values.Markus Roberts2010-07-0912-70/+17
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | * Replaced 2 occurances of def (.*) begin (.*) = Integer\((.*)\) return \2 rescue ArgumentError \2 = nil end if \2 = (.*) return \2 else return false end end with 2 Examples: The code: def validuser?(value) begin number = Integer(value) return number rescue ArgumentError number = nil end if number = uid(value) return number else return false end end becomes: def validuser?(value) Integer(value) rescue uid(value) || false end The code: def validgroup?(value) begin number = Integer(value) return number rescue ArgumentError number = nil end if number = gid(value) return number else return false end end becomes: def validgroup?(value) Integer(value) rescue gid(value) || false end * Replaced 28 occurances of return (.*?) if (.*) return (.*) with 3 Examples: The code: return send(options[:mode]) if [:rdoc, :trac, :markdown].include?(options[:mode]) return other becomes: return[:rdoc, :trac, :markdown].include?(options[:mode]) ? send(options[:mode]) : other The code: return true if known_resource_types.definition(name) return false becomes: return(known_resource_types.definition(name) ? true : false) The code: return :rest if request.protocol == 'https' return Puppet::FileBucket::File.indirection.terminus_class becomes: return(request.protocol == 'https' ? :rest : Puppet::FileBucket::File.indirection.terminus_class) * Replaced no occurances of return (.*?) unless (.*) return (.*) with * Replaced 7 occurances of if (.*) (.*[^:])false else \2true end with 3 Examples: The code: if RUBY_PLATFORM == "i386-mswin32" InstallOptions.ri = false else InstallOptions.ri = true end becomes: InstallOptions.ri = RUBY_PLATFORM != "i386-mswin32" The code: if options[:references].length > 1 with_contents = false else with_contents = true end becomes: with_contents = options[:references].length <= 1 The code: if value == false or value == "" or value == :undef return false else return true end becomes: return (value != false and value != "" and value != :undef) * Replaced 19 occurances of if (.*) (.*[^:])true else \2false end with 3 Examples: The code: if Puppet::Util::Log.level == :debug return true else return false end becomes: return Puppet::Util::Log.level == :debug The code: if satisfies?(*features) return true else return false end becomes: return !!satisfies?(*features) The code: if self.class.parsed_auth_db.has_key?(resource[:name]) return true else return false end becomes: return !!self.class.parsed_auth_db.has_key?(resource[:name]) * Replaced 1 occurance of if ([a-z_]) = (.*) (.*[^:])\1 else \3(.*) end with 1 Example: The code: if c = self.send(@subclassname, method) return c else return nil end becomes: return self.send(@subclassname, method) || nil * Replaced 2 occurances of if (.*) (.*[^:])\1 else \2false end with 2 Examples: The code: if hash[:Local] @local = hash[:Local] else @local = false end becomes: @local = hash[:Local] The code: if hash[:Local] @local = hash[:Local] else @local = false end becomes: @local = hash[:Local] * Replaced 10 occurances of if (.*) (.*[^:])(.*) else \2false end with 3 Examples: The code: if defined?(@isnamevar) return @isnamevar else return false end becomes: return defined?(@isnamevar) && @isnamevar The code: if defined?(@required) return @required else return false end becomes: return defined?(@required) && @required The code: if number = uid(value) return number else return false end becomes: return (number = uid(value)) && number * Replaced no occurances of if (.*) (.*[^:])nil else \2(true) end with * Replaced no occurances of if (.*) (.*[^:])true else \2nil end with * Replaced no occurances of if (.*) (.*[^:])\1 else \2nil end with * Replaced 23 occurances of if (.*) (.*[^:])(.*) else \2nil end with 3 Examples: The code: if node = Puppet::Node.find(hostname) env = node.environment else env = nil end becomes: env = (node = Puppet::Node.find(hostname)) ? node.environment : nil The code: if mod = Puppet::Node::Environment.new(env).module(module_name) and mod.files? return @mounts[MODULES].copy(mod.name, mod.file_directory) else return nil end becomes: return (mod = Puppet::Node::Environment.new(env).module(module_name) and mod.files?) ? @mounts[MODULES].copy(mod.name, mod.file_directory) : nil The code: if hash.include?(:CA) and hash[:CA] @ca = Puppet::SSLCertificates::CA.new() else @ca = nil end becomes: @ca = (hash.include?(:CA) and hash[:CA]) ? Puppet::SSLCertificates::CA.new() : nil
* Code smell: Line modifiers are preferred to one-line blocks.Markus Roberts2010-07-0931-327/+111
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | * Replaced 6 occurances of (while .*?) *do$ with The do is unneeded in the block header form and causes problems with the block-to-one-line transformation. 3 Examples: The code: while line = f.gets do becomes: while line = f.gets The code: while line = shadow.gets do becomes: while line = shadow.gets The code: while wrapper = zeros.pop do becomes: while wrapper = zeros.pop * Replaced 19 occurances of ((if|unless) .*?) *then$ with The then is unneeded in the block header form and causes problems with the block-to-one-line transformation. 3 Examples: The code: if f = test_files_for(failed).find { |f| failed_trace =~ Regexp.new(f) } then becomes: if f = test_files_for(failed).find { |f| failed_trace =~ Regexp.new(f) } The code: unless defined?(@spec_command) then becomes: unless defined?(@spec_command) The code: if c == ?\n then becomes: if c == ?\n * Replaced 758 occurances of ((?:if|unless|while|until) .*) (.*) end with The one-line form is preferable provided: * The condition is not used to assign a variable * The body line is not already modified * The resulting line is not too long 3 Examples: The code: if Puppet.features.libshadow? has_feature :manages_passwords end becomes: has_feature :manages_passwords if Puppet.features.libshadow? The code: unless (defined?(@current_pool) and @current_pool) @current_pool = process_zpool_data(get_pool_data) end becomes: @current_pool = process_zpool_data(get_pool_data) unless (defined?(@current_pool) and @current_pool) The code: if Puppet[:trace] puts detail.backtrace end becomes: puts detail.backtrace if Puppet[:trace]
* Code smell: Use string interpolationMarkus Roberts2010-07-0938-172/+172
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | * Replaced 83 occurances of (.*)" *[+] *([$@]?[\w_0-9.:]+?)(.to_s\b)?(?! *[*(%\w_0-9.:{\[]) with \1#{\2}" 3 Examples: The code: puts "PUPPET " + status + ": " + process + ", " + state becomes: puts "PUPPET " + status + ": " + process + ", #{state}" The code: puts "PUPPET " + status + ": #{process}" + ", #{state}" becomes: puts "PUPPET #{status}" + ": #{process}" + ", #{state}" The code: }.compact.join( "\n" ) + "\n" + t + "]\n" becomes: }.compact.join( "\n" ) + "\n#{t}" + "]\n" * Replaced 21 occurances of (.*)" *[+] *" with \1 3 Examples: The code: puts "PUPPET #{status}" + ": #{process}" + ", #{state}" becomes: puts "PUPPET #{status}" + ": #{process}, #{state}" The code: puts "PUPPET #{status}" + ": #{process}, #{state}" becomes: puts "PUPPET #{status}: #{process}, #{state}" The code: res = self.class.name + ": #{@name}" + "\n" becomes: res = self.class.name + ": #{@name}\n" * Don't use string concatenation to split lines unless they would be very long. Replaced 11 occurances of (.*)(['"]) *[+] *(['"])(.*) with 3 Examples: The code: o.define_head "The check_puppet Nagios plug-in checks that specified " + "Puppet process is running and the state file is no " + becomes: o.define_head "The check_puppet Nagios plug-in checks that specified Puppet process is running and the state file is no " + The code: o.separator "Mandatory arguments to long options are mandatory for " + "short options too." becomes: o.separator "Mandatory arguments to long options are mandatory for short options too." The code: o.define_head "The check_puppet Nagios plug-in checks that specified Puppet process is running and the state file is no " + "older than specified interval." becomes: o.define_head "The check_puppet Nagios plug-in checks that specified Puppet process is running and the state file is no older than specified interval." * Replaced no occurances of do (.*?) end with {\1} * Replaced 1488 occurances of "([^"\n]*%s[^"\n]*)" *% *(.+?)(?=$| *\b(do|if|while|until|unless|#)\b) with 20 Examples: The code: args[0].split(/\./).map do |s| "dc=%s"%[s] end.join(",") becomes: args[0].split(/\./).map do |s| "dc=#{s}" end.join(",") The code: puts "%s" % Puppet.version becomes: puts "#{Puppet.version}" The code: raise "Could not find information for %s" % node becomes: raise "Could not find information for #{node}" The code: raise Puppet::Error, "Cannot create %s: basedir %s is a file" % [dir, File.join(path)] becomes: raise Puppet::Error, "Cannot create #{dir}: basedir #{File.join(path)} is a file" The code: Puppet.err "Could not run %s: %s" % [client_class, detail] becomes: Puppet.err "Could not run #{client_class}: #{detail}" The code: raise "Could not find handler for %s" % arg becomes: raise "Could not find handler for #{arg}" The code: Puppet.err "Will not start without authorization file %s" % Puppet[:authconfig] becomes: Puppet.err "Will not start without authorization file #{Puppet[:authconfig]}" The code: raise Puppet::Error, "Could not deserialize catalog from pson: %s" % detail becomes: raise Puppet::Error, "Could not deserialize catalog from pson: #{detail}" The code: raise "Could not find facts for %s" % Puppet[:certname] becomes: raise "Could not find facts for #{Puppet[:certname]}" The code: raise ArgumentError, "%s is not readable" % path becomes: raise ArgumentError, "#{path} is not readable" The code: raise ArgumentError, "Invalid handler %s" % name becomes: raise ArgumentError, "Invalid handler #{name}" The code: debug "Executing '%s' in zone %s with '%s'" % [command, @resource[:name], str] becomes: debug "Executing '#{command}' in zone #{@resource[:name]} with '#{str}'" The code: raise Puppet::Error, "unknown cert type '%s'" % hash[:type] becomes: raise Puppet::Error, "unknown cert type '#{hash[:type]}'" The code: Puppet.info "Creating a new certificate request for %s" % Puppet[:certname] becomes: Puppet.info "Creating a new certificate request for #{Puppet[:certname]}" The code: "Cannot create alias %s: object already exists" % [name] becomes: "Cannot create alias #{name}: object already exists" The code: return "replacing from source %s with contents %s" % [metadata.source, metadata.checksum] becomes: return "replacing from source #{metadata.source} with contents #{metadata.checksum}" The code: it "should have a %s parameter" % param do becomes: it "should have a #{param} parameter" do The code: describe "when registring '%s' messages" % log do becomes: describe "when registring '#{log}' messages" do The code: paths = %w{a b c d e f g h}.collect { |l| "/tmp/iteration%stest" % l } becomes: paths = %w{a b c d e f g h}.collect { |l| "/tmp/iteration#{l}test" } The code: assert_raise(Puppet::Error, "Check '%s' did not fail on false" % check) do becomes: assert_raise(Puppet::Error, "Check '#{check}' did not fail on false") do
* Code smell: English names for special globals rather than line-noiseMarkus Roberts2010-07-095-12/+12
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | * Replaced 36 occurances of [$][?] with $CHILD_STATUS 3 Examples: The code: print "%s finished with exit code %s\n" % [host, $?.exitstatus] becomes: print "%s finished with exit code %s\n" % [host, $CHILD_STATUS.exitstatus] The code: $stderr.puts "Could not find host for PID %s with status %s" % [pid, $?.exitstatus] becomes: $stderr.puts "Could not find host for PID %s with status %s" % [pid, $CHILD_STATUS.exitstatus] The code: unless $? == 0 becomes: unless $CHILD_STATUS == 0 * Replaced 3 occurances of [$][$] with $PID 3 Examples: The code: Process.kill(:HUP, $$) if restart_requested? becomes: Process.kill(:HUP, $PID) if restart_requested? The code: if pid == $$ becomes: if pid == $PID The code: host[:name] = "!invalid.hostname.$$$" becomes: host[:name] = "!invalid.hostname.$PID$" * Replaced 7 occurances of [$]& with $MATCH 3 Examples: The code: work.slice!(0, $&.length) becomes: work.slice!(0, $MATCH.length) The code: if $& becomes: if $MATCH The code: if $& becomes: if $MATCH * Replaced 28 occurances of [$]:(?!:) with $LOAD_PATH 3 Examples: The code: sitelibdir = $:.find { |x| x =~ /site_ruby/ } becomes: sitelibdir = $LOAD_PATH.find { |x| x =~ /site_ruby/ } The code: $:.unshift "lib" becomes: $LOAD_PATH.unshift "lib" The code: $:.shift becomes: $LOAD_PATH.shift * Replaced 3 occurances of [$]! with $ERROR_INFO 3 Examples: The code: $LOG.fatal("Problem reading #{filepath}: #{$!}") becomes: $LOG.fatal("Problem reading #{filepath}: #{$ERROR_INFO}") The code: $stderr.puts "Couldn't build man pages: " + $! becomes: $stderr.puts "Couldn't build man pages: " + $ERROR_INFO The code: $stderr.puts $!.message becomes: $stderr.puts $ERROR_INFO.message * Replaced 3 occurances of ^(.*)[$]" with \1$LOADED_FEATURES 3 Examples: The code: unless $".index 'racc/parser.rb' becomes: unless $LOADED_FEATURES.index 'racc/parser.rb' The code: $".push 'racc/parser.rb' becomes: $LOADED_FEATURES.push 'racc/parser.rb' The code: $".should be_include("tmp/myfile.rb") becomes: $LOADED_FEATURES.should be_include("tmp/myfile.rb")
* Code smell: Inconsistent indentation and related formatting issuesMarkus Roberts2010-07-0931-154/+165
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | * Replaced 163 occurances of defined\? +([@a-zA-Z_.0-9?=]+) with defined?(\1) This makes detecting subsequent patterns easier. 3 Examples: The code: if ! defined? @parse_config becomes: if ! defined?(@parse_config) The code: return @option_parser if defined? @option_parser becomes: return @option_parser if defined?(@option_parser) The code: if defined? @local and @local becomes: if defined?(@local) and @local * Eliminate trailing spaces. Replaced 428 occurances of ^(.*?) +$ with \1 1 file was skipped. test/ral/providers/host/parsed.rb because 0 * Replace leading tabs with an appropriate number of spaces. Replaced 306 occurances of ^(\t+)(.*) with Tabs are not consistently expanded in all environments. * Don't arbitrarily wrap on sprintf (%) operator. Replaced 143 occurances of (.*['"] *%) +(.*) with Splitting the line does nothing to aid clarity and hinders further refactorings. 3 Examples: The code: raise Puppet::Error, "Cannot create %s: basedir %s is a file" % [dir, File.join(path)] becomes: raise Puppet::Error, "Cannot create %s: basedir %s is a file" % [dir, File.join(path)] The code: Puppet.err "Will not start without authorization file %s" % Puppet[:authconfig] becomes: Puppet.err "Will not start without authorization file %s" % Puppet[:authconfig] The code: $stderr.puts "Could not find host for PID %s with status %s" % [pid, $?.exitstatus] becomes: $stderr.puts "Could not find host for PID %s with status %s" % [pid, $?.exitstatus] * Don't break short arrays/parameter list in two. Replaced 228 occurances of (.*) +(.*) with 3 Examples: The code: puts @format.wrap(type.provider(prov).doc, :indent => 4, :scrub => true) becomes: puts @format.wrap(type.provider(prov).doc, :indent => 4, :scrub => true) The code: assert(FileTest.exists?(daily), "Did not make daily graph for %s" % type) becomes: assert(FileTest.exists?(daily), "Did not make daily graph for %s" % type) The code: assert(prov.target_object(:first).read !~ /^notdisk/, "Did not remove thing from disk") becomes: assert(prov.target_object(:first).read !~ /^notdisk/, "Did not remove thing from disk") * If arguments must wrap, treat them all equally Replaced 510 occurances of lines ending in things like ...(foo, or ...(bar(1,3), with \1 \2 3 Examples: The code: midscope.to_hash(false), becomes: assert_equal( The code: botscope.to_hash(true), becomes: # bottomscope, then checking that we see the right stuff. The code: :path => link, becomes: * Replaced 4516 occurances of ^( *)(.*) with The present code base is supposed to use four-space indentation. In some places we failed to maintain that standard. These should be fixed regardless of the 2 vs. 4 space question. 15 Examples: The code: def run_comp(cmd) puts cmd results = [] old_sync = $stdout.sync $stdout.sync = true line = [] begin open("| #{cmd}", "r") do |f| until f.eof? do c = f.getc becomes: def run_comp(cmd) puts cmd results = [] old_sync = $stdout.sync $stdout.sync = true line = [] begin open("| #{cmd}", "r") do |f| until f.eof? do c = f.getc The code: s.gsub!(/.{4}/n, '\\\\u\&') } string.force_encoding(Encoding::UTF_8) string rescue Iconv::Failure => e raise GeneratorError, "Caught #{e.class}: #{e}" end else def utf8_to_pson(string) # :nodoc: string = string.gsub(/["\\\x0-\x1f]/) { MAP[$&] } string.gsub!(/( becomes: s.gsub!(/.{4}/n, '\\\\u\&') } string.force_encoding(Encoding::UTF_8) string rescue Iconv::Failure => e raise GeneratorError, "Caught #{e.class}: #{e}" end else def utf8_to_pson(string) # :nodoc: string = string.gsub(/["\\\x0-\x1f]/) { MAP[$&] } string.gsub!(/( The code: end } rvalues: rvalue | rvalues comma rvalue { if val[0].instance_of?(AST::ASTArray) result = val[0].push(val[2]) else result = ast AST::ASTArray, :children => [val[0],val[2]] end } becomes: end } rvalues: rvalue | rvalues comma rvalue { if val[0].instance_of?(AST::ASTArray) result = val[0].push(val[2]) else result = ast AST::ASTArray, :children => [val[0],val[2]] end } The code: #passwdproc = proc { @password } keytext = @key.export( OpenSSL::Cipher::DES.new(:EDE3, :CBC), @password ) File.open(@keyfile, "w", 0400) { |f| f << keytext } becomes: # passwdproc = proc { @password } keytext = @key.export( OpenSSL::Cipher::DES.new(:EDE3, :CBC), @password ) File.open(@keyfile, "w", 0400) { |f| f << keytext } The code: end def to_manifest "%s { '%s':\n%s\n}" % [self.type.to_s, self.name, @params.collect { |p, v| if v.is_a? Array " #{p} => [\'#{v.join("','")}\']" else " #{p} => \'#{v}\'" end }.join(",\n") becomes: end def to_manifest "%s { '%s':\n%s\n}" % [self.type.to_s, self.name, @params.collect { |p, v| if v.is_a? Array " #{p} => [\'#{v.join("','")}\']" else " #{p} => \'#{v}\'" end }.join(",\n") The code: via the augeas tool. Requires: - augeas to be installed (http://www.augeas.net) - ruby-augeas bindings Sample usage with a string:: augeas{\"test1\" : context => \"/files/etc/sysconfig/firstboot\", changes => \"set RUN_FIRSTBOOT YES\", becomes: via the augeas tool. Requires: - augeas to be installed (http://www.augeas.net) - ruby-augeas bindings Sample usage with a string:: augeas{\"test1\" : context => \"/files/etc/sysconfig/firstboot\", changes => \"set RUN_FIRSTBOOT YES\", The code: names.should_not be_include("root") end describe "when generating a purgeable resource" do it "should be included in the generated resources" do Puppet::Type.type(:host).stubs(:instances).returns [@purgeable_resource] @resources.generate.collect { |r| r.ref }.should include(@purgeable_resource.ref) end end describe "when the instance's do not have an ensure property" do becomes: names.should_not be_include("root") end describe "when generating a purgeable resource" do it "should be included in the generated resources" do Puppet::Type.type(:host).stubs(:instances).returns [@purgeable_resource] @resources.generate.collect { |r| r.ref }.should include(@purgeable_resource.ref) end end describe "when the instance's do not have an ensure property" do The code: describe "when the instance's do not have an ensure property" do it "should not be included in the generated resources" do @no_ensure_resource = Puppet::Type.type(:exec).new(:name => '/usr/bin/env echo') Puppet::Type.type(:host).stubs(:instances).returns [@no_ensure_resource] @resources.generate.collect { |r| r.ref }.should_not include(@no_ensure_resource.ref) end end describe "when the instance's ensure property does not accept absent" do it "should not be included in the generated resources" do @no_absent_resource = Puppet::Type.type(:service).new(:name => 'foobar') becomes: describe "when the instance's do not have an ensure property" do it "should not be included in the generated resources" do @no_ensure_resource = Puppet::Type.type(:exec).new(:name => '/usr/bin/env echo') Puppet::Type.type(:host).stubs(:instances).returns [@no_ensure_resource] @resources.generate.collect { |r| r.ref }.should_not include(@no_ensure_resource.ref) end end describe "when the instance's ensure property does not accept absent" do it "should not be included in the generated resources" do @no_absent_resource = Puppet::Type.type(:service).new(:name => 'foobar') The code: func = nil assert_nothing_raised do func = Puppet::Parser::AST::Function.new( :name => "template", :ftype => :rvalue, :arguments => AST::ASTArray.new( :children => [stringobj(template)] ) becomes: func = nil assert_nothing_raised do func = Puppet::Parser::AST::Function.new( :name => "template", :ftype => :rvalue, :arguments => AST::ASTArray.new( :children => [stringobj(template)] ) The code: assert( @store.allowed?("hostname.madstop.com", "192.168.1.50"), "hostname not allowed") assert( ! @store.allowed?("name.sub.madstop.com", "192.168.0.50"), "subname name allowed") becomes: assert( @store.allowed?("hostname.madstop.com", "192.168.1.50"), "hostname not allowed") assert( ! @store.allowed?("name.sub.madstop.com", "192.168.0.50"), "subname name allowed") The code: assert_nothing_raised { server = Puppet::Network::Handler.fileserver.new( :Local => true, :Config => false ) } becomes: assert_nothing_raised { server = Puppet::Network::Handler.fileserver.new( :Local => true, :Config => false ) } The code: 'yay', { :failonfail => false, :uid => @user.uid, :gid => @user.gid } ).returns('output') output = Puppet::Util::SUIDManager.run_and_capture 'yay', @user.uid, @user.gid becomes: 'yay', { :failonfail => false, :uid => @user.uid, :gid => @user.gid } ).returns('output') output = Puppet::Util::SUIDManager.run_and_capture 'yay', @user.uid, @user.gid The code: ).times(1) pkg.provider.expects( :aptget ).with( '-y', '-q', 'remove', 'faff' becomes: ).times(1) pkg.provider.expects( :aptget ).with( '-y', '-q', 'remove', 'faff' The code: johnny one two billy three four\n" # Just parse and generate, to make sure it's isomorphic. assert_nothing_raised do assert_equal(text, @parser.to_file(@parser.parse(text)), "parsing was not isomorphic") end end def test_valid_attrs becomes: johnny one two billy three four\n" # Just parse and generate, to make sure it's isomorphic. assert_nothing_raised do assert_equal(text, @parser.to_file(@parser.parse(text)), "parsing was not isomorphic") end end def test_valid_attrs The code: "testing", :onboolean => [true, "An on bool"], :string => ["a string", "A string arg"] ) result = [] should = [] assert_nothing_raised("Add args failed") do @config.addargs(result) end @config.each do |name, element| becomes: "testing", :onboolean => [true, "An on bool"], :string => ["a string", "A string arg"] ) result = [] should = [] assert_nothing_raised("Add args failed") do @config.addargs(result) end @config.each do |name, element|