{"action":"created","issue":{"url":"https://api.github.com/repos/ogcaizu/ogc-poc1/issues/2","repository_url":"https://api.github.com/repos/ogcaizu/ogc-poc1","labels_url":"https://api.github.com/repos/ogcaizu/ogc-poc1/issues/2/labels{/name}","comments_url":"https://api.github.com/repos/ogcaizu/ogc-poc1/issues/2/comments","events_url":"https://api.github.com/repos/ogcaizu/ogc-poc1/issues/2/events","html_url":"https://github.com/ogcaizu/ogc-poc1/pull/2","id":371773033,"node_id":"MDExOlB1bGxSZXF1ZXN0MjI0MTQ1ODI2","number":2,"title":"Feature/slack","user":{"login":"urabe0225","id":4556975,"node_id":"MDQ6VXNlcjQ1NTY5NzU=","avatar_url":"https://avatars2.githubusercontent.com/u/4556975?v=4","gravatar_id":"","url":"https://api.github.com/users/urabe0225","html_url":"https://github.com/urabe0225","followers_url":"https://api.github.com/users/urabe0225/followers","following_url":"https://api.github.com/users/urabe0225/following{/other_user}","gists_url":"https://api.github.com/users/urabe0225/gists{/gist_id}","starred_url":"https://api.github.com/users/urabe0225/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/urabe0225/subscriptions","organizations_url":"https://api.github.com/users/urabe0225/orgs","repos_url":"https://api.github.com/users/urabe0225/repos","events_url":"https://api.github.com/users/urabe0225/events{/privacy}","received_events_url":"https://api.github.com/users/urabe0225/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2018-10-19T01:29:54Z","updated_at":"2018-10-19T04:29:46Z","closed_at":null,"author_association":"NONE","pull_request":{"url":"https://api.github.com/repos/ogcaizu/ogc-poc1/pulls/2","html_url":"https://github.com/ogcaizu/ogc-poc1/pull/2","diff_url":"https://github.com/ogcaizu/ogc-poc1/pull/2.diff","patch_url":"https://github.com/ogcaizu/ogc-poc1/pull/2.patch"},"body":"request.postのモック化など修正しました"},"comment":{"url":"https://api.github.com/repos/ogcaizu/ogc-poc1/issues/comments/431241421","html_url":"https://github.com/ogcaizu/ogc-poc1/pull/2#issuecomment-431241421","issue_url":"https://api.github.com/repos/ogcaizu/ogc-poc1/issues/2","id":431241421,"node_id":"MDEyOklzc3VlQ29tbWVudDQzMTI0MTQyMQ==","user":{"login":"nmatsui","id":296122,"node_id":"MDQ6VXNlcjI5NjEyMg==","avatar_url":"https://avatars1.githubusercontent.com/u/296122?v=4","gravatar_id":"","url":"https://api.github.com/users/nmatsui","html_url":"https://github.com/nmatsui","followers_url":"https://api.github.com/users/nmatsui/followers","following_url":"https://api.github.com/users/nmatsui/following{/other_user}","gists_url":"https://api.github.com/users/nmatsui/gists{/gist_id}","starred_url":"https://api.github.com/users/nmatsui/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nmatsui/subscriptions","organizations_url":"https://api.github.com/users/nmatsui/orgs","repos_url":"https://api.github.com/users/nmatsui/repos","events_url":"https://api.github.com/users/nmatsui/events{/privacy}","received_events_url":"https://api.github.com/users/nmatsui/received_events","type":"User","site_admin":false},"created_at":"2018-10-19T04:29:46Z","updated_at":"2018-10-19T04:29:46Z","author_association":"COLLABORATOR","body":"# src/slack.py\r\n## 仕様上の問題\r\n* Pepper側の都合上、destinationsの `name` はASCII文字しか入らないようになった\r\n    * Pepperが胸のタブレットに表示するのは単なる画像のため、destinationsの `name` は使っていない\r\n* この仕様変更に伴い、Slackへ通知する際にも渡された `name` を適切な日本語名に変換する必要がある\r\n    * twelve factor app的に考えれば、 `name` と日本語名の対応表は環境変数経由で渡したほうが良い（プログラムを変更せずとも、kubernetesのyamlファイルを変更して再デプロイすれば修正できる）\r\n    * 今回は実証実験であり目的地の追加変更は発生しないため、プログラム中に固定的に宣言してしまうという手もある（良い実装とは言えないが、動作はする）\r\n    * あるいはdestination serviceに手を入れて、今の `name` だけでなく `display_name` フィールドを追加して、それを `send_message_to_slack()` に渡すという実装も考えられる。修正点は多いが、「目的地」に関する情報は全てdestination serviceに集約できるため美しい。\r\n\r\n## 実装上の問題\r\n* `r = requests.post(webhook, data=json.dumps(payload_dic))` はexceptionをraiseする可能性がある（接続先ドメインが存在しない、接続先のポートが空いて無い、接続先からResponseが返ってこない・・・）\r\n    * → `requests.post` もtry文の中で実行するべき\r\n    * → この場合、 `ConnectionError` や `Timeout` 等のエラーも発生するので、それぞれ個別にexceptするか `requests.exceptions.RequestException` をexceptするべき\r\n> http://docs.python-requests.org/en/master/user/quickstart/#errors-and-exceptions\r\n\r\n## 運用上の問題\r\n* slackにメッセージを送信できなかった場合、exceptionをraiseしてシステムを停止させているが、この運用で問題ないのか、システムの非機能要件として合意する必要がある\r\n    * → 今回は実証実験なのでシステム停止でも良いが、普通のシステム開発であれば、システムが停止した場合何らかの人的対処が必要になるので、ログ監視でアラートを上げ、発生した場合の運用手順を定義しておく必要がある\r\n\r\n## その他\r\n* 同じエラーでエラーログを複数回出力しないほうが良い（ログを分析してエラーの発生回数をチェックする際に重複カウントしてしまう）\r\n\r\n* 成功した場合の `logger.info(r)` は、DEBUGレベルで良いのでは（通常稼働しているシステムではINFO以上のログをログ監視することが多いので、あとで分析や障害解析などで必要になる以外の情報をINFOで出すのは避けたほうが良い）\r\n\r\n----\r\n# tests/test_slack.py\r\n* `requests.post` に想定通りの引数（urlとdata）が渡されているのかを確認する正常系のテストが無い\r\n    * `mock_post.assert_called_once_with()` 等でassertする\r\n* 上記のように、ConnectionErrorやTimeout等のExceptionがraiseされる場合のテストが無い\r\n* 引数のwebhookやroomが想定外だった場合のテストが無い\r\n    * webhookがNoneだった場合の挙動（実際にNoneだった場合、 `MissingSchema` がraiseされると思う）\r\n    * webhookが正しいURL文字列ではない場合の挙動\r\n    * 上述のように `name` と日本語名の対応表を定義している場合、roomがNoneの場合や想定外の値の場合の挙動はテストしておく必要がある\r\n\r\n## その他\r\n* (pythonのloadpathの設定次第ではあるけれども）receptionのルートディレクトリがloadpathされている前提であれば、 `from src.slack import send_message_to_slack` になるはず"}}
{"actor":{"display_login":"nmatsui"}}