From c6def3c3142947d8468d770b3b4582008bcc4fef Mon Sep 17 00:00:00 2001 From: Romuald Conty Date: Mon, 21 Dec 2020 15:38:53 +0100 Subject: [PATCH 1/3] CLI: Force thor to exit with a correct exit status Long-time issues on Thor related to exit status code cause msync to return a inappropriate exit code. As example: https://github.com/erikhuda/thor/issues/244 --- features/cli.feature | 6 +++++- lib/modulesync/cli.rb | 4 +++- lib/modulesync/cli/thor.rb | 24 ++++++++++++++++++++++++ 3 files changed, 32 insertions(+), 2 deletions(-) create mode 100644 lib/modulesync/cli/thor.rb diff --git a/features/cli.feature b/features/cli.feature index f4ad61ed..111de29f 100644 --- a/features/cli.feature +++ b/features/cli.feature @@ -4,18 +4,22 @@ Feature: CLI Scenario: When passing no arguments to the msync command When I run `msync` And the output should match /Commands:/ + Then the exit status should be 1 Scenario: When passing invalid arguments to the msync update command When I run `msync update` And the output should match /No value provided for required option/ + Then the exit status should be 1 Scenario: When passing invalid arguments to the msync hook command When I run `msync hook` And the output should match /Commands:/ + Then the exit status should be 1 - Scenario: When running the help subcommand + Scenario: When running the help command When I run `msync help` And the output should match /Commands:/ + Then the exit status should be 0 Scenario: When overriding a setting from the config file on the command line Given a puppet module "puppet-test" from "fakenamespace" diff --git a/lib/modulesync/cli.rb b/lib/modulesync/cli.rb index 72602219..bd733e67 100644 --- a/lib/modulesync/cli.rb +++ b/lib/modulesync/cli.rb @@ -1,10 +1,12 @@ require 'thor' + require 'modulesync' +require 'modulesync/cli/thor' require 'modulesync/constants' require 'modulesync/util' module ModuleSync - class CLI + module CLI def self.defaults @defaults ||= Util.symbolize_keys(Util.parse_config(Constants::MODULESYNC_CONF_FILE)) end diff --git a/lib/modulesync/cli/thor.rb b/lib/modulesync/cli/thor.rb new file mode 100644 index 00000000..0b8566b8 --- /dev/null +++ b/lib/modulesync/cli/thor.rb @@ -0,0 +1,24 @@ +require 'thor' +require 'modulesync/cli' + +module ModuleSync + module CLI + # Workaround some, still unfixed, Thor behaviors + # + # This class extends ::Thor class to + # - exit with status code sets to `1` on Thor failure (e.g. missing required option) + # - exit with status code sets to `1` when user calls `msync` (or a subcommand) without required arguments + class Thor < ::Thor + desc '_invalid_command_call', 'Invalid command', hide: true + def _invalid_command_call + self.class.new.help + exit 1 + end + default_task :_invalid_command_call + + def self.exit_on_failure? + true + end + end + end +end From 0b98f485d13861de316ba2526121e1ef48a3eb0e Mon Sep 17 00:00:00 2001 From: Romuald Conty Date: Mon, 21 Dec 2020 15:55:37 +0100 Subject: [PATCH 2/3] Fix the exit status when running update without all requirements --- features/update/bad_context.feature | 26 ++++++++++++++++++++++++++ lib/modulesync.rb | 8 ++++---- 2 files changed, 30 insertions(+), 4 deletions(-) create mode 100644 features/update/bad_context.feature diff --git a/features/update/bad_context.feature b/features/update/bad_context.feature new file mode 100644 index 00000000..b4a4c16b --- /dev/null +++ b/features/update/bad_context.feature @@ -0,0 +1,26 @@ +Feature: Run `msync update` without a good context + + Scenario: Run `msync update` without any module + Given a directory named "moduleroot" + When I run `msync update --message "In a bad context"` + Then the exit status should be 1 + And the stderr should contain: + """ + No modules found + """ + + Scenario: Run `msync update` without the "moduleroot" directory + Given a basic setup with a puppet module "puppet-test" from "fakenamespace" + When I run `msync update --message "In a bad context"` + Then the exit status should be 1 + And the stderr should contain "moduleroot" + + Scenario: Run `msync update` without commit message + Given a basic setup with a puppet module "puppet-test" from "fakenamespace" + And a directory named "moduleroot" + When I run `msync update` + Then the exit status should be 1 + And the stderr should contain: + """ + No value provided for required option "--message" + """ diff --git a/lib/modulesync.rb b/lib/modulesync.rb index 101d4197..9efed9c7 100644 --- a/lib/modulesync.rb +++ b/lib/modulesync.rb @@ -39,10 +39,10 @@ def self.find_template_files(local_template_dir) .collect { |p| p.chomp('.erb') } .to_a else - $stdout.puts "#{local_template_dir} does not exist." \ + $stderr.puts "#{local_template_dir} does not exist." \ ' Check that you are working in your module configs directory or' \ ' that you have passed in the correct directory with -c.' - exit + exit 1 end end @@ -53,9 +53,9 @@ def self.relative_names(file_list, path) def self.managed_modules(config_file, filter, negative_filter) managed_modules = Util.parse_config(config_file) if managed_modules.empty? - $stdout.puts "No modules found in #{config_file}." \ + $stderr.puts "No modules found in #{config_file}." \ ' Check that you specified the right :configs directory and :managed_modules_conf file.' - exit + exit 1 end managed_modules.select! { |m| m =~ Regexp.new(filter) } unless filter.nil? managed_modules.reject! { |m| m =~ Regexp.new(negative_filter) } unless negative_filter.nil? From 463384fb1fa4ac90a9391fc4d0a209e1c558a6ee Mon Sep 17 00:00:00 2001 From: Romuald Conty Date: Mon, 21 Dec 2020 15:59:09 +0100 Subject: [PATCH 3/3] Tests: Fix `update` scenarios that lack of context/option These tests were successful for wrong reasons! This commit fixes the relevant scenarios, by setting correctly the requirements (ie. message and moduleroot) --- features/update.feature | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/features/update.feature b/features/update.feature index dd096cf7..f7d94c79 100644 --- a/features/update.feature +++ b/features/update.feature @@ -430,7 +430,8 @@ Feature: update Scenario: Pulling a module that already exists in the modules directory Given a basic setup with a puppet module "puppet-test" from "fakenamespace" - When I run `msync update` + And a directory named "moduleroot" + When I run `msync update --message "First update run"` Then the exit status should be 0 And the puppet module "puppet-test" from "fakenamespace" should have no commits made by "Aruba" Given a file named "config_defaults.yml" with: @@ -455,10 +456,10 @@ Feature: update """ And the puppet module "puppet-test" from "fakenamespace" should have no commits made by "Aruba" - Scenario: When running update with no changes + Scenario: When running update without changes Given a basic setup with a puppet module "puppet-test" from "fakenamespace" And a directory named "moduleroot" - When I run `msync update` + When I run `msync update --message "Running without changes"` Then the exit status should be 0 And the puppet module "puppet-test" from "fakenamespace" should have no commits made by "Aruba"