(PUP-11993) Style/RedundantBegin

This commit enables the Style/RedundantBegin cop and fixes 815
autocorrectable offenses.

Author: AriaXLi

.rubocop_todo.yml

@@ -646,10 +646,6 @@ Style/OptionalBooleanParameter:
 Style/PreferredHashMethods:
   Enabled: false
 
-# This cop supports safe auto-correction (--auto-correct).
-Style/RedundantBegin:
-  Enabled: false
-
 # This cop supports safe auto-correction (--auto-correct).
 Style/RedundantCondition:
   Exclude:

ext/windows/service/daemon.rb

@@ -75,25 +75,23 @@ def service_main(*argsv)
 
     service = self
     @run_thread = Thread.new do
-      begin
-        while service.running? do
-          runinterval = service.parse_runinterval(ruby_puppet_cmd)
-
-          if service.state == RUNNING or service.state == IDLE
-            service.log_notice("Executing agent with arguments: #{args}")
-            pid = Process.create(:command_line => "#{ruby_puppet_cmd} agent --onetime #{args}", :creation_flags => CREATE_NEW_CONSOLE).process_id
-            service.log_debug("Process created: #{pid}")
-          else
-            service.log_debug("Service is paused.  Not invoking Puppet agent")
-          end
-
-          service.log_debug("Service worker thread waiting for #{runinterval} seconds")
-          sleep(runinterval)
-          service.log_debug('Service worker thread woken up')
+      while service.running? do
+        runinterval = service.parse_runinterval(ruby_puppet_cmd)
+
+        if service.state == RUNNING or service.state == IDLE
+          service.log_notice("Executing agent with arguments: #{args}")
+          pid = Process.create(:command_line => "#{ruby_puppet_cmd} agent --onetime #{args}", :creation_flags => CREATE_NEW_CONSOLE).process_id
+          service.log_debug("Process created: #{pid}")
+        else
+          service.log_debug("Service is paused.  Not invoking Puppet agent")
         end
-      rescue Exception => e
-        service.log_exception(e)
+
+        service.log_debug("Service worker thread waiting for #{runinterval} seconds")
+        sleep(runinterval)
+        service.log_debug('Service worker thread woken up')
       end
+    rescue Exception => e
+      service.log_exception(e)
     end
     @run_thread.join
   rescue Exception => e
@@ -142,20 +140,18 @@ def log(msg, level)
   end
 
   def report_windows_event(type, id, message)
-    begin
-      eventlog = nil
-      eventlog = Puppet::Util::Windows::EventLog.open("Puppet")
-      eventlog.report_event(
-        :event_type => type, # EVENTLOG_ERROR_TYPE, etc
-        :event_id => id, # 0x01 or 0x02, 0x03 etc.
-        :data => message # "the message"
-      )
-    rescue Exception
-      # Ignore all errors
-    ensure
-      unless eventlog.nil?
-        eventlog.close
-      end
+    eventlog = nil
+    eventlog = Puppet::Util::Windows::EventLog.open("Puppet")
+    eventlog.report_event(
+      :event_type => type, # EVENTLOG_ERROR_TYPE, etc
+      :event_id => id, # 0x01 or 0x02, 0x03 etc.
+      :data => message # "the message"
+    )
+  rescue Exception
+    # Ignore all errors
+  ensure
+    unless eventlog.nil?
+      eventlog.close
     end
   end
 
@@ -192,24 +188,22 @@ def parse_log_level(puppet_path, cmdline_debug)
   private
 
   def load_env(base_dir)
-    begin
-      # ENV that uses backward slashes
-      ENV['FACTER_env_windows_installdir'] = base_dir.tr('/', '\\')
-      ENV['PL_BASEDIR'] = base_dir.tr('/', '\\')
-      ENV['PUPPET_DIR'] = File.join(base_dir, 'puppet').tr('/', '\\')
-      ENV['OPENSSL_CONF'] = File.join(base_dir, 'puppet', 'ssl', 'openssl.cnf').tr('/', '\\')
-      ENV['SSL_CERT_DIR'] = File.join(base_dir, 'puppet', 'ssl', 'certs').tr('/', '\\')
-      ENV['SSL_CERT_FILE'] = File.join(base_dir, 'puppet', 'ssl', 'cert.pem').tr('/', '\\')
-      ENV['Path'] = [
-        File.join(base_dir, 'puppet', 'bin'),
-        File.join(base_dir, 'bin'),
-      ].join(';').tr('/', '\\') + ';' + ENV.fetch('Path', nil)
-
-      # ENV that uses forward slashes
-      ENV['RUBYLIB'] = "#{File.join(base_dir, 'puppet', 'lib')};#{ENV.fetch('RUBYLIB', nil)}"
-    rescue => e
-      log_exception(e)
-    end
+    # ENV that uses backward slashes
+    ENV['FACTER_env_windows_installdir'] = base_dir.tr('/', '\\')
+    ENV['PL_BASEDIR'] = base_dir.tr('/', '\\')
+    ENV['PUPPET_DIR'] = File.join(base_dir, 'puppet').tr('/', '\\')
+    ENV['OPENSSL_CONF'] = File.join(base_dir, 'puppet', 'ssl', 'openssl.cnf').tr('/', '\\')
+    ENV['SSL_CERT_DIR'] = File.join(base_dir, 'puppet', 'ssl', 'certs').tr('/', '\\')
+    ENV['SSL_CERT_FILE'] = File.join(base_dir, 'puppet', 'ssl', 'cert.pem').tr('/', '\\')
+    ENV['Path'] = [
+      File.join(base_dir, 'puppet', 'bin'),
+      File.join(base_dir, 'bin'),
+    ].join(';').tr('/', '\\') + ';' + ENV.fetch('Path', nil)
+
+    # ENV that uses forward slashes
+    ENV['RUBYLIB'] = "#{File.join(base_dir, 'puppet', 'lib')};#{ENV.fetch('RUBYLIB', nil)}"
+  rescue => e
+    log_exception(e)
   end
 end
 

lib/puppet/application.rb

@@ -481,12 +481,10 @@ def handle_logdest_arg(arg)
     Puppet[:logdest] = arg
 
     logdest.each do |dest|
-      begin
-        Puppet::Util::Log.newdestination(dest)
-        options[:setdest] = true
-      rescue => detail
-        Puppet.log_and_raise(detail, _("Could not set logdest to %{dest}.") % { dest: arg })
-      end
+      Puppet::Util::Log.newdestination(dest)
+      options[:setdest] = true
+    rescue => detail
+      Puppet.log_and_raise(detail, _("Could not set logdest to %{dest}.") % { dest: arg })
     end
   end
 

lib/puppet/application/device.rb

@@ -263,115 +263,114 @@ def main
       end
       devices.collect do |_devicename, device|
         # TODO when we drop support for ruby < 2.5 we can remove the extra block here
-        begin
-          device_url = URI.parse(device.url)
-          # Handle nil scheme & port
-          scheme = "#{device_url.scheme}://" if device_url.scheme
-          port = ":#{device_url.port}" if device_url.port
-
-          # override local $vardir and $certname
-          Puppet[:ssldir] = ::File.join(Puppet[:deviceconfdir], device.name, 'ssl')
-          Puppet[:confdir] = ::File.join(Puppet[:devicedir], device.name)
-          Puppet[:libdir] = options[:libdir] || ::File.join(Puppet[:devicedir], device.name, 'lib')
-          Puppet[:vardir] = ::File.join(Puppet[:devicedir], device.name)
-          Puppet[:certname] = device.name
-          ssl_context = nil
-
-          # create device directory under $deviceconfdir
-          Puppet::FileSystem.dir_mkpath(Puppet[:ssldir]) unless Puppet::FileSystem.dir_exist?(Puppet[:ssldir])
-
-          # this will reload and recompute default settings and create device-specific sub vardir
-          Puppet.settings.use :main, :agent, :ssl
-
-          # Workaround for PUP-8736: store ssl certs outside the cache directory to prevent accidental removal and keep the old path as symlink
-          optssldir = File.join(Puppet[:confdir], 'ssl')
-          Puppet::FileSystem.symlink(Puppet[:ssldir], optssldir) unless Puppet::FileSystem.exist?(optssldir)
-
-          unless options[:resource] || options[:facts] || options[:apply]
-            # Since it's too complicated to fix properly in the default settings, we workaround for PUP-9642 here.
-            # See https://github.com/puppetlabs/puppet/pull/7483#issuecomment-483455997 for details.
-            # This has to happen after `settings.use` above, so the directory is created and before `setup_host` below, where the SSL
-            # routines would fail with access errors
-            if Puppet.features.root? && !Puppet::Util::Platform.windows?
-              user = Puppet::Type.type(:user).new(name: Puppet[:user]).exists? ? Puppet[:user] : nil
-              group = Puppet::Type.type(:group).new(name: Puppet[:group]).exists? ? Puppet[:group] : nil
-              Puppet.debug("Fixing perms for #{user}:#{group} on #{Puppet[:confdir]}")
-              FileUtils.chown(user, group, Puppet[:confdir]) if user || group
-            end
 
-            ssl_context = setup_context
+        device_url = URI.parse(device.url)
+        # Handle nil scheme & port
+        scheme = "#{device_url.scheme}://" if device_url.scheme
+        port = ":#{device_url.port}" if device_url.port
+
+        # override local $vardir and $certname
+        Puppet[:ssldir] = ::File.join(Puppet[:deviceconfdir], device.name, 'ssl')
+        Puppet[:confdir] = ::File.join(Puppet[:devicedir], device.name)
+        Puppet[:libdir] = options[:libdir] || ::File.join(Puppet[:devicedir], device.name, 'lib')
+        Puppet[:vardir] = ::File.join(Puppet[:devicedir], device.name)
+        Puppet[:certname] = device.name
+        ssl_context = nil
+
+        # create device directory under $deviceconfdir
+        Puppet::FileSystem.dir_mkpath(Puppet[:ssldir]) unless Puppet::FileSystem.dir_exist?(Puppet[:ssldir])
+
+        # this will reload and recompute default settings and create device-specific sub vardir
+        Puppet.settings.use :main, :agent, :ssl
+
+        # Workaround for PUP-8736: store ssl certs outside the cache directory to prevent accidental removal and keep the old path as symlink
+        optssldir = File.join(Puppet[:confdir], 'ssl')
+        Puppet::FileSystem.symlink(Puppet[:ssldir], optssldir) unless Puppet::FileSystem.exist?(optssldir)
+
+        unless options[:resource] || options[:facts] || options[:apply]
+          # Since it's too complicated to fix properly in the default settings, we workaround for PUP-9642 here.
+          # See https://github.com/puppetlabs/puppet/pull/7483#issuecomment-483455997 for details.
+          # This has to happen after `settings.use` above, so the directory is created and before `setup_host` below, where the SSL
+          # routines would fail with access errors
+          if Puppet.features.root? && !Puppet::Util::Platform.windows?
+            user = Puppet::Type.type(:user).new(name: Puppet[:user]).exists? ? Puppet[:user] : nil
+            group = Puppet::Type.type(:group).new(name: Puppet[:group]).exists? ? Puppet[:group] : nil
+            Puppet.debug("Fixing perms for #{user}:#{group} on #{Puppet[:confdir]}")
+            FileUtils.chown(user, group, Puppet[:confdir]) if user || group
+          end
+
+          ssl_context = setup_context
 
-            unless options[:libdir]
-              Puppet.override(ssl_context: ssl_context) do
-                Puppet::Configurer::PluginHandler.new.download_plugins(env) if Puppet::Configurer.should_pluginsync?
-              end
+          unless options[:libdir]
+            Puppet.override(ssl_context: ssl_context) do
+              Puppet::Configurer::PluginHandler.new.download_plugins(env) if Puppet::Configurer.should_pluginsync?
             end
           end
+        end
 
-          # this inits the device singleton, so that the facts terminus
-          # and the various network_device provider can use it
-          Puppet::Util::NetworkDevice.init(device)
-
-          if options[:resource]
-            type, name = parse_args(command_line.args)
-            Puppet.info _("retrieving resource: %{resource} from %{target} at %{scheme}%{url_host}%{port}%{url_path}") % { resource: type, target: device.name, scheme: scheme, url_host: device_url.host, port: port, url_path: device_url.path }
-            resources = find_resources(type, name)
-            if options[:to_yaml]
-              data = resources.map do |resource|
-                resource.prune_parameters(:parameters_to_include => @extra_params).to_hiera_hash
-              end.inject(:merge!)
-              text = YAML.dump(type.downcase => data)
-            else
-              text = resources.map do |resource|
-                resource.prune_parameters(:parameters_to_include => @extra_params).to_manifest.force_encoding(Encoding.default_external)
-              end.join("\n")
-            end
-            (puts text)
-            0
-          elsif options[:facts]
-            Puppet.info _("retrieving facts from %{target} at %{scheme}%{url_host}%{port}%{url_path}") % { resource: type, target: device.name, scheme: scheme, url_host: device_url.host, port: port, url_path: device_url.path }
-            remote_facts = Puppet::Node::Facts.indirection.find(name, :environment => env)
-            # Give a proper name to the facts
-            remote_facts.name = remote_facts.values['clientcert']
-            renderer = Puppet::Network::FormatHandler.format(:console)
-            puts renderer.render(remote_facts)
-            0
-          elsif options[:apply]
-            # avoid reporting to server
-            Puppet::Transaction::Report.indirection.terminus_class = :yaml
-            Puppet::Resource::Catalog.indirection.cache_class = nil
-
-            require_relative '../../puppet/application/apply'
-            begin
-              Puppet[:node_terminus] = :plain
-              Puppet[:catalog_terminus] = :compiler
-              Puppet[:catalog_cache_terminus] = nil
-              Puppet[:facts_terminus] = :network_device
-              Puppet.override(:network_device => true) do
-                Puppet::Application::Apply.new(Puppet::Util::CommandLine.new('puppet', ["apply", options[:apply]])).run_command
-              end
-            end
+        # this inits the device singleton, so that the facts terminus
+        # and the various network_device provider can use it
+        Puppet::Util::NetworkDevice.init(device)
+
+        if options[:resource]
+          type, name = parse_args(command_line.args)
+          Puppet.info _("retrieving resource: %{resource} from %{target} at %{scheme}%{url_host}%{port}%{url_path}") % { resource: type, target: device.name, scheme: scheme, url_host: device_url.host, port: port, url_path: device_url.path }
+          resources = find_resources(type, name)
+          if options[:to_yaml]
+            data = resources.map do |resource|
+              resource.prune_parameters(:parameters_to_include => @extra_params).to_hiera_hash
+            end.inject(:merge!)
+            text = YAML.dump(type.downcase => data)
           else
-            Puppet.info _("starting applying configuration to %{target} at %{scheme}%{url_host}%{port}%{url_path}") % { target: device.name, scheme: scheme, url_host: device_url.host, port: port, url_path: device_url.path }
-
-            overrides = {}
-            overrides[:ssl_context] = ssl_context if ssl_context
-            Puppet.override(overrides) do
-              configurer = Puppet::Configurer.new
-              configurer.run(:network_device => true, :pluginsync => false)
+            text = resources.map do |resource|
+              resource.prune_parameters(:parameters_to_include => @extra_params).to_manifest.force_encoding(Encoding.default_external)
+            end.join("\n")
+          end
+          (puts text)
+          0
+        elsif options[:facts]
+          Puppet.info _("retrieving facts from %{target} at %{scheme}%{url_host}%{port}%{url_path}") % { resource: type, target: device.name, scheme: scheme, url_host: device_url.host, port: port, url_path: device_url.path }
+          remote_facts = Puppet::Node::Facts.indirection.find(name, :environment => env)
+          # Give a proper name to the facts
+          remote_facts.name = remote_facts.values['clientcert']
+          renderer = Puppet::Network::FormatHandler.format(:console)
+          puts renderer.render(remote_facts)
+          0
+        elsif options[:apply]
+          # avoid reporting to server
+          Puppet::Transaction::Report.indirection.terminus_class = :yaml
+          Puppet::Resource::Catalog.indirection.cache_class = nil
+
+          require_relative '../../puppet/application/apply'
+          begin
+            Puppet[:node_terminus] = :plain
+            Puppet[:catalog_terminus] = :compiler
+            Puppet[:catalog_cache_terminus] = nil
+            Puppet[:facts_terminus] = :network_device
+            Puppet.override(:network_device => true) do
+              Puppet::Application::Apply.new(Puppet::Util::CommandLine.new('puppet', ["apply", options[:apply]])).run_command
             end
           end
-        rescue => detail
-          Puppet.log_exception(detail)
-          # If we rescued an error, then we return 1 as the exit code
-          1
-        ensure
-          Puppet[:libdir] = libdir
-          Puppet[:vardir] = vardir
-          Puppet[:confdir] = confdir
-          Puppet[:ssldir] = ssldir
-          Puppet[:certname] = certname
+        else
+          Puppet.info _("starting applying configuration to %{target} at %{scheme}%{url_host}%{port}%{url_path}") % { target: device.name, scheme: scheme, url_host: device_url.host, port: port, url_path: device_url.path }
+
+          overrides = {}
+          overrides[:ssl_context] = ssl_context if ssl_context
+          Puppet.override(overrides) do
+            configurer = Puppet::Configurer.new
+            configurer.run(:network_device => true, :pluginsync => false)
+          end
         end
+      rescue => detail
+        Puppet.log_exception(detail)
+        # If we rescued an error, then we return 1 as the exit code
+        1
+      ensure
+        Puppet[:libdir] = libdir
+        Puppet[:vardir] = vardir
+        Puppet[:confdir] = confdir
+        Puppet[:ssldir] = ssldir
+        Puppet[:certname] = certname
       end
     end
 

lib/puppet/configurer.rb

@@ -748,14 +748,12 @@ def retrieve_new_catalog(facts, query_options)
   end
 
   def download_plugins(remote_environment_for_plugins)
-    begin
-      @handler.download_plugins(remote_environment_for_plugins)
-    rescue Puppet::Error => detail
-      if !Puppet[:ignore_plugin_errors] && Puppet[:usecacheonfailure]
-        @running_failure = true
-      else
-        raise detail
-      end
+    @handler.download_plugins(remote_environment_for_plugins)
+  rescue Puppet::Error => detail
+    if !Puppet[:ignore_plugin_errors] && Puppet[:usecacheonfailure]
+      @running_failure = true
+    else
+      raise detail
     end
   end
 end