-
Notifications
You must be signed in to change notification settings - Fork 13
here is a change that enabled sentry entries to be sent when there is output in the stderr but exit code is still 0 #28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,6 +54,15 @@ | |
| default=False, | ||
| help='Report to Sentry even if the task has succeeded', | ||
| ) | ||
|
|
||
| parser.add_argument( | ||
| '--report-stderr', | ||
| action='store_true', | ||
| default=False, | ||
| help='Report when stderr is not empty' | ||
| ) | ||
|
|
||
|
|
||
| parser.add_argument( | ||
| 'cmd', | ||
| nargs=REMAINDER, | ||
|
|
@@ -125,7 +134,8 @@ def run(args=argv[1:]): | |
| string_max_length=opts.string_max_length, | ||
| quiet=opts.quiet, | ||
| extra=_extra_from_env(environ), | ||
| report_all=opts.report_all | ||
| report_all=opts.report_all, | ||
| report_stderr=opts.report_stderr | ||
| ) | ||
| sys.exit(runner.run()) | ||
| else: | ||
|
|
@@ -135,13 +145,15 @@ def run(args=argv[1:]): | |
|
|
||
|
|
||
| class CommandReporter(object): | ||
| def __init__(self, cmd, dsn, string_max_length, quiet=False, extra=None, report_all=False): | ||
| def __init__(self, cmd, dsn, string_max_length, quiet=False, extra=None, report_all=False, report_stderr=False): | ||
| self.dsn = dsn | ||
| self.command = cmd | ||
| self.string_max_length = string_max_length | ||
| self.quiet = quiet | ||
| self.extra = {} | ||
| self.report_all = report_all | ||
| self.report_stderr = report_stderr | ||
| self.level = logging.INFO | ||
| if extra is not None: | ||
| self.extra = extra | ||
|
|
||
|
|
@@ -152,22 +164,32 @@ def run(self): | |
| with TemporaryFile() as stderr: | ||
| try: | ||
| exit_status = call(self.command, stdout=stdout, stderr=stderr) | ||
| last_lines_stdout = self._get_last_lines(stdout) | ||
| last_lines_stderr = self._get_last_lines(stderr) | ||
| except CommandNotFoundError as exc: | ||
| last_lines_stdout = '' | ||
| last_lines_stderr = str(exc) | ||
| exit_status = 127 # http://www.tldp.org/LDP/abs/html/exitcodes.html | ||
| else: | ||
| last_lines_stdout = self._get_last_lines(stdout) | ||
| last_lines_stderr = self._get_last_lines(stderr) | ||
|
|
||
| if self.report_all or exit_status != 0: | ||
| elapsed = int((time() - start) * 1000) | ||
| self.report(exit_status, last_lines_stdout, last_lines_stderr, elapsed) | ||
| elapsed = int((time() - start) * 1000) | ||
|
|
||
| if not self.quiet: | ||
| sys.stdout.write(last_lines_stdout) | ||
| sys.stderr.write(last_lines_stderr) | ||
|
|
||
| if self.report_all: | ||
| self.report(exit_status, last_lines_stdout ,last_lines_stderr,elapsed) | ||
| return exit_status | ||
|
|
||
| if exit_status != 0: | ||
| self.level = logging.ERROR | ||
| self.report(exit_status, last_lines_stdout, last_lines_stderr, elapsed) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. won't this report twice if it exits with anything but 0 and has stderr because there's no |
||
|
|
||
| if self.report_stderr and last_lines_stderr: | ||
| self.level = logging.ERROR | ||
| self.report(exit_status, last_lines_stdout, last_lines_stderr, elapsed) | ||
| return exit_status | ||
|
|
||
| return exit_status | ||
|
|
||
| def report(self, exit_status, last_lines_stdout, last_lines_stderr, elapsed): | ||
|
|
@@ -176,10 +198,10 @@ def report(self, exit_status, last_lines_stdout, last_lines_stderr, elapsed): | |
|
|
||
| if exit_status == 0: | ||
| message = "Command \"%s\" has succeeded" % (self.command,) | ||
| log_level = logging.INFO | ||
| log_level = self.level | ||
| else: | ||
| message = "Command \"%s\" has failed" % (self.command,) | ||
| log_level = logging.ERROR | ||
| log_level = self.level | ||
|
|
||
| client = Client(transport=HTTPTransport, dsn=self.dsn, string_max_length=self.string_max_length) | ||
| extra = self.extra.copy() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,11 +36,11 @@ def test_command_reporter_catches_invalid_commands(ClientMock, sys_mock): | |
| time_spent=mock.ANY, | ||
| data=mock.ANY, | ||
| extra={ | ||
| 'command': command, | ||
| 'exit_status': expected_exit_code, | ||
| "last_lines_stdout": expected_stdout, | ||
| "last_lines_stderr": mock.ANY, | ||
| }) | ||
| 'command':command, | ||
| 'exit_status':expected_exit_code, | ||
| "last_lines_stdout":expected_stdout, | ||
| "last_lines_stderr":mock.ANY, | ||
| }) | ||
| assert exit_code == expected_exit_code | ||
| sys_mock.stdout.write.assert_called_with(expected_stdout) | ||
| # python 3 exception `FileNotFoundError` has additional content compared to python 2's `OSError` | ||
|
|
@@ -80,11 +80,11 @@ def test_command_reporter_keeps_stdout_and_stderr(ClientMock, sys_mock): | |
| time_spent=mock.ANY, | ||
| data=mock.ANY, | ||
| extra={ | ||
| 'command': command, | ||
| 'exit_status': 2, | ||
| "last_lines_stdout": "test-out", | ||
| "last_lines_stderr": "test-err", | ||
| }) | ||
| 'command':command, | ||
| 'exit_status':2, | ||
| "last_lines_stdout":"test-out", | ||
| "last_lines_stderr":"test-err", | ||
| }) | ||
|
|
||
|
|
||
| @mock.patch('cron_sentry.runner.sys') | ||
|
|
@@ -111,11 +111,11 @@ def test_reports_correctly_to_with_long_messages_but_trims_stdout_and_stderr(Cli | |
| time_spent=mock.ANY, | ||
| data=mock.ANY, | ||
| extra={ | ||
| 'command': command, | ||
| 'exit_status': 2, | ||
| "last_lines_stdout": expected_stdout, | ||
| "last_lines_stderr": expected_stderr, | ||
| }) | ||
| 'command':command, | ||
| 'exit_status':2, | ||
| "last_lines_stdout":expected_stdout, | ||
| "last_lines_stderr":expected_stderr, | ||
| }) | ||
|
|
||
|
|
||
| @mock.patch('cron_sentry.runner.sys') | ||
|
|
@@ -131,6 +131,7 @@ def test_command_line_should_support_command_args_without_double_dashes(CommandR | |
| string_max_length=DEFAULT_STRING_MAX_LENGTH, | ||
| quiet=False, | ||
| report_all=False, | ||
| report_stderr=False, | ||
| extra={}, | ||
| ) | ||
|
|
||
|
|
@@ -148,14 +149,16 @@ def test_command_line_should_support_command_with_double_dashes(CommandReporterM | |
| string_max_length=DEFAULT_STRING_MAX_LENGTH, | ||
| quiet=False, | ||
| report_all=False, | ||
| report_stderr=False, | ||
| extra={}, | ||
| ) | ||
|
|
||
|
|
||
| @mock.patch('cron_sentry.runner.sys') | ||
| @mock.patch('argparse._sys') | ||
| @mock.patch('cron_sentry.runner.CommandReporter') | ||
| def test_should_display_help_text_and_exit_with_1_if_no_command_is_specified(CommandReporterMock, argparse_sys, cron_sentry_sys): | ||
| def test_should_display_help_text_and_exit_with_1_if_no_command_is_specified(CommandReporterMock, argparse_sys, | ||
| cron_sentry_sys): | ||
| command = [] | ||
| run(command) | ||
|
|
||
|
|
@@ -175,14 +178,13 @@ def test_exit_status_code_should_be_preserved(ClientMock, sys_mock): | |
| sys_mock.exit.assert_called_with(123) | ||
|
|
||
|
|
||
|
|
||
| @mock.patch('cron_sentry.runner.sys') | ||
| @mock.patch('cron_sentry.runner.Client') | ||
| def test_should_trim_stdout_and_stderr_based_on_command_line(ClientMock, sys_mock): | ||
| command = [ | ||
| '--dsn', 'http://testdsn', | ||
| '--max-message-length', '100', | ||
| sys.executable, '-c', """ | ||
| '--dsn', 'http://testdsn', | ||
| '--max-message-length', '100', | ||
| sys.executable, '-c', """ | ||
| import sys | ||
| sys.stdout.write("a" * 20000 + "end") | ||
| sys.stderr.write("b" * 20000 + "end") | ||
|
|
@@ -204,19 +206,20 @@ def test_should_trim_stdout_and_stderr_based_on_command_line(ClientMock, sys_moc | |
| time_spent=mock.ANY, | ||
| data=mock.ANY, | ||
| extra={ | ||
| 'command': mock.ANY, | ||
| 'exit_status': mock.ANY, | ||
| "last_lines_stdout": expected_stdout, | ||
| "last_lines_stderr": expected_stderr, | ||
| }) | ||
| 'command':mock.ANY, | ||
| 'exit_status':mock.ANY, | ||
| "last_lines_stdout":expected_stdout, | ||
| "last_lines_stderr":expected_stderr, | ||
| }) | ||
|
|
||
|
|
||
| @mock.patch('cron_sentry.runner.sys') | ||
| @mock.patch('cron_sentry.runner.Client') | ||
| def test_should_suppress_stdout_and_stderr_based_on_command_line(ClientMock, sys_mock): | ||
| command = [ | ||
| '--dsn', 'http://testdsn', | ||
| '--quiet', | ||
| sys.executable, '-c', """ | ||
| '--dsn', 'http://testdsn', | ||
| '--quiet', | ||
| sys.executable, '-c', """ | ||
| import sys | ||
| sys.stdout.write("a" * 100 + "end") | ||
| sys.stderr.write("b" * 100 + "end") | ||
|
|
@@ -238,11 +241,11 @@ def test_should_suppress_stdout_and_stderr_based_on_command_line(ClientMock, sys | |
| time_spent=mock.ANY, | ||
| data=mock.ANY, | ||
| extra={ | ||
| 'command': mock.ANY, | ||
| 'exit_status': mock.ANY, | ||
| "last_lines_stdout": expected_stdout, | ||
| "last_lines_stderr": expected_stderr, | ||
| }) | ||
| 'command':mock.ANY, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you revert all stylistic changes, please? they are not related to the pull request. |
||
| 'exit_status':mock.ANY, | ||
| "last_lines_stdout":expected_stdout, | ||
| "last_lines_stderr":expected_stderr, | ||
| }) | ||
|
|
||
|
|
||
| @mock.patch('cron_sentry.runner.sys') | ||
|
|
@@ -268,10 +271,48 @@ def test_extra_data_via_env_vars_should_go_to_sentry(ClientMock, sys_mock): | |
| time_spent=mock.ANY, | ||
| data=mock.ANY, | ||
| extra={ | ||
| 'command': mock.ANY, | ||
| 'exit_status': mock.ANY, | ||
| 'last_lines_stdout': mock.ANY, | ||
| 'last_lines_stderr': mock.ANY, | ||
| 'secret1': 'hello', | ||
| 'secret2': 'world', | ||
| }) | ||
| 'command':mock.ANY, | ||
| 'exit_status':mock.ANY, | ||
| 'last_lines_stdout':mock.ANY, | ||
| 'last_lines_stderr':mock.ANY, | ||
| 'secret1':'hello', | ||
| 'secret2':'world', | ||
| }) | ||
|
|
||
|
|
||
| @mock.patch('cron_sentry.runner.sys') | ||
| @mock.patch('cron_sentry.runner.Client') | ||
| def test_report_stderr(ClientMock, sys_mock): | ||
|
|
||
| command = ['--dsn', | ||
| 'http://testdsn', | ||
| '--report-stderr', | ||
| sys.executable, '-c', """ | ||
| import sys | ||
| sys.stdout.write("a" * 100 + "end") | ||
| sys.stderr.write("b" * 100 + "end") | ||
| sys.exit(0) | ||
| """ | ||
| ] | ||
| run(command) | ||
|
|
||
| expected_stdout = "a" * 100 + "end" | ||
| expected_stderr = "b" * 100 + "end" | ||
|
|
||
| assert sys_mock.stdout.write.called | ||
| assert sys_mock.stderr.write.called | ||
|
|
||
|
|
||
| client = ClientMock() | ||
| client.captureMessage.assert_called_with( | ||
| mock.ANY, | ||
| level=logging.ERROR, | ||
| time_spent=mock.ANY, | ||
| data=mock.ANY, | ||
| extra={ | ||
| 'command':mock.ANY, | ||
| 'exit_status':0, | ||
| 'last_lines_stdout':mock.ANY, | ||
| 'last_lines_stderr':expected_stderr | ||
| } | ||
| ) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason to move the
last_lines_stdoutandlast_lines_stderrlines to inside thetry, @rogersprint? The reason they were outside originally is that onlycall()should be raisingCommandNotFoundError.