{"action":"created","issue":{"url":"https://api.github.com/repos/m-lab/mlab-ns/issues/86","repository_url":"https://api.github.com/repos/m-lab/mlab-ns","labels_url":"https://api.github.com/repos/m-lab/mlab-ns/issues/86/labels{/name}","comments_url":"https://api.github.com/repos/m-lab/mlab-ns/issues/86/comments","events_url":"https://api.github.com/repos/m-lab/mlab-ns/issues/86/events","html_url":"https://github.com/m-lab/mlab-ns/pull/86","id":149802228,"number":86,"title":"Added logic to StatusUpdateHandler to deal with blank slice and sliver statuses.","user":{"login":"fernandalavalle","id":3836867,"avatar_url":"https://avatars.githubusercontent.com/u/3836867?v=3","gravatar_id":"","url":"https://api.github.com/users/fernandalavalle","html_url":"https://github.com/fernandalavalle","followers_url":"https://api.github.com/users/fernandalavalle/followers","following_url":"https://api.github.com/users/fernandalavalle/following{/other_user}","gists_url":"https://api.github.com/users/fernandalavalle/gists{/gist_id}","starred_url":"https://api.github.com/users/fernandalavalle/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/fernandalavalle/subscriptions","organizations_url":"https://api.github.com/users/fernandalavalle/orgs","repos_url":"https://api.github.com/users/fernandalavalle/repos","events_url":"https://api.github.com/users/fernandalavalle/events{/privacy}","received_events_url":"https://api.github.com/users/fernandalavalle/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":4,"created_at":"2016-04-20T15:23:39Z","updated_at":"2016-04-21T19:14:31Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/m-lab/mlab-ns/pulls/86","html_url":"https://github.com/m-lab/mlab-ns/pull/86","diff_url":"https://github.com/m-lab/mlab-ns/pull/86.diff","patch_url":"https://github.com/m-lab/mlab-ns/pull/86.patch"},"body":"\n\n<!-- Reviewable:start -->\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"35\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/m-lab/mlab-ns/86)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/m-lab/mlab-ns/issues/comments/213075777","html_url":"https://github.com/m-lab/mlab-ns/pull/86#issuecomment-213075777","issue_url":"https://api.github.com/repos/m-lab/mlab-ns/issues/86","id":213075777,"user":{"login":"fernandalavalle","id":3836867,"avatar_url":"https://avatars.githubusercontent.com/u/3836867?v=3","gravatar_id":"","url":"https://api.github.com/users/fernandalavalle","html_url":"https://github.com/fernandalavalle","followers_url":"https://api.github.com/users/fernandalavalle/followers","following_url":"https://api.github.com/users/fernandalavalle/following{/other_user}","gists_url":"https://api.github.com/users/fernandalavalle/gists{/gist_id}","starred_url":"https://api.github.com/users/fernandalavalle/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/fernandalavalle/subscriptions","organizations_url":"https://api.github.com/users/fernandalavalle/orgs","repos_url":"https://api.github.com/users/fernandalavalle/repos","events_url":"https://api.github.com/users/fernandalavalle/events{/privacy}","received_events_url":"https://api.github.com/users/fernandalavalle/received_events","type":"User","site_admin":false},"created_at":"2016-04-21T19:14:31Z","updated_at":"2016-04-21T19:14:31Z","body":"\n\n\n\nReview status: 0 of 4 files reviewed at latest revision, 21 unresolved discussions.\n\n---\n\n*[server/mlabns/handlers/update.py, line 338 \\[r1\\]](https://reviewable.io:443/reviews/m-lab/mlab-ns/86#-KFoUrs3-_2WZwR2sPKQ:-KFou5BiQatSYkhRGo69:231803702) ([raw file](https://github.com/m-lab/mlab-ns/blob/3c1b247cb28bd5e6e7a5d0e878fe66743fb980b6/server/mlabns/handlers/update.py#L338)):*\nOkay, yes I know. I thought about how this was digging a deeper hole but from what we talked about evaluating whether a service name is ipv4 or ipv6 is something that should be native to NagiosSliceInfo which this would have access to, so I thought it wasn't too terrible. \n\nI added this check because from what I understood we don't expect blank statuses for any service on ipv4 but we expect several services to always return blank slice statuses for ipv6. I didn't want the ipv4 to fail silently. \n\n---\n\n*[server/mlabns/handlers/update.py, line 433 \\[r1\\]](https://reviewable.io:443/reviews/m-lab/mlab-ns/86#-KFoJWWfkGINpk8Quo-H:-KFt8xGDfpeWtUg9CEw8:-140720236) ([raw file](https://github.com/m-lab/mlab-ns/blob/3c1b247cb28bd5e6e7a5d0e878fe66743fb980b6/server/mlabns/handlers/update.py#L433)):*\nSo that's not totally the same. What you suggested just checks for blank slice statuses (the whole thing), and the current solution checks for blank sliver statuses (each line) too. \n\nYou think its reasonable to assume that if the slice status isn't empty or blank, we won't get blank lines for sliver statuses? Seems fine to me, I just want to check.\n\n---\n\n*[server/mlabns/tests/test_update.py, line 104 \\[r1\\]](https://reviewable.io:443/reviews/m-lab/mlab-ns/86#-KFoO34ib5gTgoZEpZ78:-KFtFJmuBJfgSATGN-Fm:391671373) ([raw file](https://github.com/m-lab/mlab-ns/blob/3c1b247cb28bd5e6e7a5d0e878fe66743fb980b6/server/mlabns/tests/test_update.py#L104)):*\nThe indentations mess up parsing. Do you still prefer the triple-quote with awkward dedent?\n\n---\n\n*[server/mlabns/tests/test_update.py, line 124 \\[r1\\]](https://reviewable.io:443/reviews/m-lab/mlab-ns/86#-KFoQ5CRCvoFmK2IoSes:-KFtaL-F7JPtG_LD4lVX:429485859) ([raw file](https://github.com/m-lab/mlab-ns/blob/3c1b247cb28bd5e6e7a5d0e878fe66743fb980b6/server/mlabns/tests/test_update.py#L124)):*\nYou can't use this format for the tests that need parse_sliver_tool_status to raise exceptions, can you? \n\nIs self consistency less valuable than less indirection? Should I only implement this for the tests currently using lambda?\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/m-lab/mlab-ns/86)*\n<!-- Sent from Reviewable.io -->\n"}}