Notify when back to normal #93

takonomura · 2018-01-19T04:50:26Z

Notify when retried job finished.

ganmacs · 2018-01-19T06:39:52Z

@cookpad/dev-infra review please

eisuke · 2018-01-19T06:43:40Z

lib/autoload/kuroko2/workflow/notifier/hipchat.rb

@@ -73,7 +73,7 @@ def notify_critical
        end

        def notify_finished
-          if @definition.hipchat_notify_finished?
+          if @definition.hipchat_notify_finished? || @instance.notify_back_to_normal?
            message = build_message(level: 'SUCCESS', text: message_builder.finished_text)


I think that it would be better to change message text.

eisuke · 2018-01-19T06:44:30Z

lib/autoload/kuroko2/workflow/notifier/slack.rb

@@ -80,7 +80,7 @@ def notify_critical
        end

        def notify_finished
-          if @definition.hipchat_notify_finished?
+          if @definition.hipchat_notify_finished? || @instance.notify_back_to_normal?
            send_attachment_message_to_slack(
              level: 'SUCCESS',
              text: message_builder.finished_text,


(ditto) I think that it would be better to change message text.

eisuke · 2018-01-19T06:49:54Z

db/migrate/030_add_notify_back_to_normal.rb

@@ -0,0 +1,6 @@
+class AddNotifyBackToNormal < ActiveRecord::Migration[5.1]
+  def change
+    add_column :job_definitions, :notify_back_to_normal, :boolean


How about adding after: :notify_cancellation ?

eisuke · 2018-01-19T07:05:46Z

lib/autoload/kuroko2/workflow/engine.rb

@@ -25,7 +25,7 @@ def retry(token)
          node = extract_node(token)

          message = "(token #{token.uuid}) Retry current node: '#{node.type}: #{node.option}'"
-          token.job_instance.update_column(:error_at, nil)
+          token.job_instance.update_columns(error_at: nil, retrying: true)


If some tasks exists,

eg.

my_runner: first_task my_runner: second_task

a first task has an error.

a first task retried and finished successfully.

Notified back to normal

a second task finished successfully

Notified back to normal

The No.5 notification is not necessary.

Further, the fork task and parallel_fork task are more complicated. It will not work we want with this implementation.
https://github.com/cookpad/kuroko2/blob/master/docs/tasks.md#fork
https://github.com/cookpad/kuroko2/blob/master/docs/tasks.md#parallel_fork

I think that it is better to set retrying status in tokens.context.

Thanks, I fixed it in c5246f2.

eisuke · 2018-01-22T07:07:26Z

lib/autoload/kuroko2/workflow/engine.rb

@@ -119,6 +120,7 @@ def process_next(node, token)
          token.mark_as_finished
          unless token.parent
            token.job_instance.touch(:finished_at)
+            Notifier.notify(:back_to_normal, token.job_instance) if token.context['RETRYING']


Don't you need deleting the RETRYING context?

token will be destroyed at line 125. So I decided there was no need to delete the RETRYING context.

That's right (I forgot). That seems to be OK.

Oh..., execuse me.
https://github.com/cookpad/kuroko2/pull/93/files#diff-f7b019ffd787516ae6ba13584edcd76dR121
It is necessary to put it outside the above conditions.

eisuke

LGTM

eagletmt · 2018-01-22T09:47:13Z

lib/autoload/kuroko2/workflow/engine.rb

@@ -119,6 +120,7 @@ def process_next(node, token)
          token.mark_as_finished
          unless token.parent
            token.job_instance.touch(:finished_at)
+            Notifier.notify(:back_to_normal, token.job_instance) if token.context['RETRYING']
            Notifier.notify(:finished, token.job_instance)


When retried, should "finished" notification be sent?

It is necessary for when the job has enabled Notify all event to Slack/Hipchat/Webhook and disabled Notify when back to normal option.

Sending both "back to normal" and "finish" notification with checking" notification is duplicated when the job is enabled both "Notify all event to Slack/Hipchat/Webhook" and " Notify when back to normal", isn't it?

Sorry, I understand. I fixed it in 370977b.

eagletmt · 2018-01-22T09:49:38Z

lib/autoload/kuroko2/workflow/notifier/concerns/chat_message_builder.rb

@@ -20,6 +20,10 @@ def launched_text
            "Launched '#{@definition.name}'"
          end

+          def back_to_normal_text
+            "Back to normal the current task in '#{@definition.name}'"


"The task in '#{@definition.name}' was successfully retried" is easier to understand I think.

eisuke · 2018-01-22T09:59:31Z

lib/autoload/kuroko2/workflow/engine.rb

@@ -119,6 +120,7 @@ def process_next(node, token)
          token.mark_as_finished
          unless token.parent
            token.job_instance.touch(:finished_at)
+            Notifier.notify(:back_to_normal, token.job_instance) if token.context['RETRYING']


Oh..., execuse me.
https://github.com/cookpad/kuroko2/pull/93/files#diff-f7b019ffd787516ae6ba13584edcd76dR121
It is necessary to put it outside the above conditions.

ganmacs

LGTM

ganmacs · 2018-01-26T02:06:29Z

Thanks 👍

Add option to notify when back to normal

f6fa0e0

eisuke requested changes Jan 19, 2018

View reviewed changes

takonomura added 3 commits January 19, 2018 16:26

Change position of column notify_back_to_normal

f6fb69d

Disallow null for column notify_back_to_normal

3b0d971

Fix for multiple tasks, and change message text

c5246f2

takonomura force-pushed the feature/notify-back-to-normal branch from db9d444 to c5246f2 Compare January 19, 2018 08:56

eisuke reviewed Jan 22, 2018

View reviewed changes

eisuke approved these changes Jan 22, 2018

View reviewed changes

eagletmt reviewed Jan 22, 2018

View reviewed changes

eisuke requested changes Jan 22, 2018

View reviewed changes

takonomura added 4 commits January 23, 2018 09:45

Change message for back to normal

d341bc2

Fix condition for back to normal

3ddf3d4

Fix duplication of notification

370977b

Notify after finished the job instad of the token

54e4ab4

takonomura changed the title ~~Add option to notify when back to normal~~ Notify when back to normal Jan 24, 2018

ganmacs approved these changes Jan 25, 2018

View reviewed changes

eisuke approved these changes Jan 25, 2018

View reviewed changes

ganmacs merged commit b61d296 into cookpad:master Jan 26, 2018

takonomura deleted the feature/notify-back-to-normal branch January 29, 2018 06:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notify when back to normal #93

Notify when back to normal #93

takonomura commented Jan 19, 2018 •

edited

Loading

ganmacs commented Jan 19, 2018

eisuke Jan 19, 2018

eisuke Jan 19, 2018

eisuke Jan 19, 2018

eisuke Jan 19, 2018

takonomura Jan 22, 2018

eisuke Jan 22, 2018

takonomura Jan 22, 2018

eisuke Jan 22, 2018

eisuke Jan 22, 2018

eisuke left a comment

eagletmt Jan 22, 2018

takonomura Jan 23, 2018

ganmacs Jan 23, 2018

takonomura Jan 23, 2018

eagletmt Jan 22, 2018

eisuke Jan 22, 2018

ganmacs left a comment

ganmacs commented Jan 26, 2018

Notify when back to normal #93

Notify when back to normal #93

Conversation

takonomura commented Jan 19, 2018 • edited Loading

ganmacs commented Jan 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eisuke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ganmacs left a comment

Choose a reason for hiding this comment

ganmacs commented Jan 26, 2018

takonomura commented Jan 19, 2018 •

edited

Loading