From: Jude N Date: Thu, 21 Dec 2017 14:39:22 +0000 (-0500) Subject: Syncing up with what's in production :-/ X-Git-Url: https://pwan.org/git/?p=margebot.git;a=commitdiff_plain;h=010dc0cc40c579e431f6d327838c4c88631ef2a2 Syncing up with what's in production :-/ - the webhook doesn't report a new MR if it's already tracking it --- diff --git a/.pylintrc b/.pylintrc index 4d85818..0de8463 100644 --- a/.pylintrc +++ b/.pylintrc @@ -59,7 +59,7 @@ confidence= # --enable=similarities". If you want to run only the classes checker, but have # no Warning level messages displayed, use"--disable=all --enable=classes # --disable=W" -disable=import-star-module-level,old-octal-literal,oct-method,print-statement,unpacking-in-except,parameter-unpacking,backtick,old-raise-syntax,old-ne-operator,long-suffix,dict-view-method,dict-iter-method,metaclass-assignment,next-method-called,raising-string,indexing-exception,raw_input-builtin,long-builtin,file-builtin,execfile-builtin,coerce-builtin,cmp-builtin,buffer-builtin,basestring-builtin,apply-builtin,filter-builtin-not-iterating,using-cmp-argument,useless-suppression,range-builtin-not-iterating,suppressed-message,no-absolute-import,old-division,cmp-method,reload-builtin,zip-builtin-not-iterating,intern-builtin,unichr-builtin,reduce-builtin,standarderror-builtin,unicode-builtin,xrange-builtin,coerce-method,delslice-method,getslice-method,setslice-method,input-builtin,round-builtin,hex-method,nonzero-method,map-builtin-not-iterating,line-too-long,no-name-in-module,input-error,missing-super-argument,no-self-use,too-many-locals,too-many-branches,import-error,locally-disabled,locally-enabled,too-many-ancestors,useless-super-delegation,logging-format-interpolation +disable=import-star-module-level,old-octal-literal,oct-method,print-statement,unpacking-in-except,parameter-unpacking,backtick,old-raise-syntax,old-ne-operator,long-suffix,dict-view-method,dict-iter-method,metaclass-assignment,next-method-called,raising-string,indexing-exception,raw_input-builtin,long-builtin,file-builtin,execfile-builtin,coerce-builtin,cmp-builtin,buffer-builtin,basestring-builtin,apply-builtin,filter-builtin-not-iterating,using-cmp-argument,useless-suppression,range-builtin-not-iterating,suppressed-message,no-absolute-import,old-division,cmp-method,reload-builtin,zip-builtin-not-iterating,intern-builtin,unichr-builtin,reduce-builtin,standarderror-builtin,unicode-builtin,xrange-builtin,coerce-method,delslice-method,getslice-method,setslice-method,input-builtin,round-builtin,hex-method,nonzero-method,map-builtin-not-iterating,line-too-long,no-name-in-module,input-error,missing-super-argument,no-self-use,too-many-locals,too-many-branches,import-error,locally-disabled,locally-enabled,too-many-ancestors,useless-super-delegation,logging-format-interpolation,too-many-statements [REPORTS] diff --git a/plugins/marge.py b/plugins/marge.py index 968ef7a..42e42c3 100755 --- a/plugins/marge.py +++ b/plugins/marge.py @@ -137,23 +137,25 @@ class Marge(BotPlugin, CrontabMixin): target_project_id = request['object_attributes']['target_project_id'] iid = request['object_attributes']['iid'] + mr_id = request['object_attributes']['id'] - msg_template = "New Review: {} has opened a new {}MR: \"{}\"\n{}/merge_requests/{}" + msg_template = "Hi there ! {} has opened a new {}MR: \"{}\"\n{}/merge_requests/{}" msg = msg_template.format(author_name, wip, title, url, iid) - for a_room in rooms.split(','): - if self.config: - self.send(self.build_identifier(a_room + '@' + self.chatroom_host), msg) - if 'OPEN_MRS' not in self.keys(): empty_dict = {} self['OPEN_MRS'] = empty_dict open_mrs = self['OPEN_MRS'] - self.log.info("webhook: Saving ({}, {}, {})".format(target_project_id, iid, rooms)) - open_mrs[(target_project_id, id, rooms)] = True - self['OPEN_MRS'] = open_mrs + if (target_project_id, mr_id, rooms) not in open_mrs: + for a_room in rooms.split(','): + if self.config: + self.send(self.build_identifier(a_room + '@' + self.chatroom_host), msg) + + self.log.info("webhook: Saving ({}, {}, {})".format(target_project_id, mr_id, rooms)) + open_mrs[(target_project_id, mr_id, rooms)] = True + self['OPEN_MRS'] = open_mrs return "OK" def mr_status_msg(self, a_mr, author=None): @@ -162,17 +164,19 @@ class Marge(BotPlugin, CrontabMixin): """ self.log.info("mr_status_msg: a_mr: {}".format(a_mr)) + # Only weed out MRs less than the soak time for the crontab output (where author==None) now = datetime.now(timezone.utc) creation_time = parser.parse(a_mr['created_at'], tzinfos=tzutc) - self.log.info("times: {}, {}, {}".format(creation_time, self.soak_delta, now)) - if creation_time + self.soak_delta > now: - project = a_mr['project'] - iid = a_mr['iid'] - soak_hours = self.config['CRONTAB_SOAK_HOURS'] - info_template = "skipping: MR <{},{}> was opened less than {} hours ago" - info_msg = info_template.format(project, iid, soak_hours) - self.log.info(info_msg) - return None + if not author: + self.log.info("times: {}, {}, {}".format(creation_time, self.soak_delta, now)) + if creation_time + self.soak_delta > now: + project_id = a_mr['target_project_id'] + mr_id = a_mr['id'] + soak_hours = self.config['CRONTAB_SOAK_HOURS'] + info_template = "skipping: MR <{},{}> was opened less than {} hours ago" + info_msg = info_template.format(project_id, mr_id, soak_hours) + self.log.info(info_msg) + return None str_open_since = deltastr(now - creation_time) @@ -246,19 +250,19 @@ class Marge(BotPlugin, CrontabMixin): # Let's walk through the MRs we've seen already: open_mrs = self['OPEN_MRS'] - for (project, iid, notify_rooms) in open_mrs: + for (project_id, mr_id, notify_rooms) in open_mrs: - # Lookup the MR from the project/iid - a_mr = self.gitlab.getmergerequest(project, iid) + # Lookup the MR from the project/id + a_mr = self.gitlab.getmergerequest(project_id, mr_id) - self.log.info("a_mr: {} {} {} {}".format(project, iid, notify_rooms, a_mr['state'])) + self.log.info("a_mr: {} {} {} {}".format(project_id, mr_id, notify_rooms, a_mr['state'])) # If the MR is no longer open, skip to the next MR, # and don't include this MR in the next check if 'opened' not in a_mr['state']: continue else: - still_open_mrs[(project, iid, notify_rooms)] = True + still_open_mrs[(project_id, mr_id, notify_rooms)] = True msg_tuple = self.mr_status_msg(a_mr) if msg_tuple is None: @@ -283,7 +287,7 @@ class Marge(BotPlugin, CrontabMixin): room_msg = "\n".join(msgs) if self.config: - msg_template = "These MRs need some attention:{}\n" + msg_template = "These MRs need some attention:\n{}\n" msg_template += "You can get an updated list with the !reviews command." to_room = a_room + '@' + self.config['CHATROOM_HOST'] msg = msg_template.format(room_msg) @@ -330,17 +334,17 @@ class Marge(BotPlugin, CrontabMixin): msg = "" still_open_mrs = {} open_mrs = self['OPEN_MRS'] - for (project, iid, notify_rooms) in open_mrs: + for (project, mr_id, notify_rooms) in open_mrs: - # Lookup the MR from the project/iid - a_mr = self.gitlab.getmergerequest(project, iid) + # Lookup the MR from the project/id + a_mr = self.gitlab.getmergerequest(project, mr_id) # If the MR is no longer open, skip to the next MR, # and don't include this MR in the next check if 'opened' not in a_mr['state']: continue else: - still_open_mrs[(project, iid, notify_rooms)] = True + still_open_mrs[(project, mr_id, notify_rooms)] = True msg_tuple = self.mr_status_msg(a_mr, author=sender_gitlab_id) if msg_tuple is None: diff --git a/tests/test_marge.py b/tests/test_marge.py index bb53f9d..8aed0ba 100644 --- a/tests/test_marge.py +++ b/tests/test_marge.py @@ -38,6 +38,10 @@ class TestMarge(object): testbot.push_message("!plugin config Webserver {'HOST': '0.0.0.0', 'PORT':3141, 'SSL': {'certificate': '', 'enabled': False, 'host': '0.0.0.0', 'key': '', 'port': 3142}}") testbot.pop_message() + def mock_get(self, key): + return {} + monkeypatch.setattr(errbot.storage.StoreMixin, '__getitem__', mock_get, raising=False) + def mock_keys(self): return ['OPEN_MRS'] monkeypatch.setattr(errbot.storage.StoreMixin, 'keys', mock_keys, raising=False) @@ -46,11 +50,6 @@ class TestMarge(object): @pytest.fixture def margebot_no_reviews(self, margebot, monkeypatch): - - def mock_get(self, key): - return {} - - monkeypatch.setattr(errbot.storage.StoreMixin, '__getitem__', mock_get, raising=False) return margebot @pytest.fixture @@ -151,6 +150,7 @@ class TestMarge(object): def mock_getmergerequest(self, project, iid): return {} monkeypatch.setattr(gitlab, 'getuser', mock_getmergerequest) + return gitlab @pytest.fixture def gitlab_one_review(self, gitlab, monkeypatch): @@ -163,6 +163,7 @@ class TestMarge(object): 'web_url': 'http://gitlab.example.com/sample/mr/2001', 'work_in_progress': False} monkeypatch.setattr(gitlab, 'getmergerequest', mock_getmergerequest) + return gitlab # ============================================================================ @@ -211,13 +212,14 @@ class TestMarge(object): 'title': 'title', 'author_id': 'author_id', 'target_project_id': 'project_id', + 'id': 'id', 'iid': 'iid'}, 'project': { 'homepage': 'url'}}) margebot.push_message("!webhook test /margebot/room1,room2 " + request) - assert 'New Review: author_id username has opened a new MR: \"title\"\nurl/merge_requests/iid' in margebot.pop_message() + assert 'Hi there ! author_id username has opened a new MR: \"title\"\nurl/merge_requests/iid' in margebot.pop_message() margebot.push_message('!reviews') - assert 'New Review: author_id username has opened a new MR: \"title\"\nurl/merge_requests/iid' in margebot.pop_message() + assert 'Hi there ! author_id username has opened a new MR: \"title\"\nurl/merge_requests/iid' in margebot.pop_message() def test_gitlab_hook_wip(self, margebot, gitlab): request = json.dumps({'object_kind': 'merge_request', @@ -227,13 +229,14 @@ class TestMarge(object): 'title': 'title', 'author_id': 'author_id', 'target_project_id': 'project_id', + 'id': 'id', 'iid': 'iid'}, 'project': { 'homepage': 'url'}}) margebot.push_message("!webhook test /margebot/room1,room2 " + request) - assert 'New Review: author_id username has opened a new WIP MR: \"title\"\nurl/merge_requests/iid' in margebot.pop_message() + assert 'Hi there ! author_id username has opened a new WIP MR: \"title\"\nurl/merge_requests/iid' in margebot.pop_message() margebot.push_message('!reviews') - assert 'New Review: author_id username has opened a new WIP MR: \"title\"\nurl/merge_requests/iid' in margebot.pop_message() + assert 'Hi there ! author_id username has opened a new WIP MR: \"title\"\nurl/merge_requests/iid' in margebot.pop_message() def test_gitlab_hook_unexpected_user(self, margebot, gitlab): request = json.dumps({'object_kind': 'merge_request', @@ -243,16 +246,17 @@ class TestMarge(object): 'title': 'title', 'author_id': '(missing)', 'target_project_id': 'project_id', + 'id': 'id', 'iid': 'iid'}, 'project': { 'homepage': 'url'}}) margebot.push_message("!webhook test /margebot/room1,room2 " + request) - assert 'New Review: (missing) has opened a new MR: \"title\"\nurl/merge_requests/iid' in margebot.pop_message() + assert 'Hi there ! (missing) has opened a new MR: \"title\"\nurl/merge_requests/iid' in margebot.pop_message() margebot.push_message('!reviews') - assert 'New Review: (missing) has opened a new MR: \"title\"\nurl/merge_requests/iid' in margebot.pop_message() + assert 'Hi there ! (missing) has opened a new MR: \"title\"\nurl/merge_requests/iid' in margebot.pop_message() - def test_gitlab_hook_unexpected_object_kind(self, margebot_no_reviews, gitlab, caplog): + def test_gitlab_hook_unexpected_object_kind(self, margebot, gitlab, caplog): request = json.dumps({'object_kind': 'not_merge_request', 'object_attributes': { 'state': 'opened', @@ -260,19 +264,20 @@ class TestMarge(object): 'title': 'title', 'author_id': 'author_id', 'target_project_id': 'project_id', + 'id': 'id', 'iid': 'iid'}, 'project': { 'homepage': 'url'}}) - margebot_no_reviews.push_message("!webhook test /margebot/room1,room2 " + request) - margebot_no_reviews.pop_message() - margebot_no_reviews.push_message('!reviews') - assert 'Hi gbin: I found no open MRs for you.' in margebot_no_reviews.pop_message() + margebot.push_message("!webhook test /margebot/room1,room2 " + request) + margebot.pop_message() + margebot.push_message('!reviews') + assert 'Hi gbin: I found no open MRs for you.' in margebot.pop_message() assert 'unexpecting object_kind: not_merge_request' in caplog.text # Has to be at end of method - def test_nothing_to_review(self, margebot_no_reviews, gitlab_no_reviews): - margebot_no_reviews.push_message('!reviews') - assert 'Hi gbin: I found no open MRs for you.' in margebot_no_reviews.pop_message() + def test_nothing_to_review(self, margebot, gitlab_no_reviews): + margebot.push_message('!reviews') + assert 'Hi gbin: I found no open MRs for you.' in margebot.pop_message() def test_get_one_open_mr(self, margebot_one_review, gitlab_one_review): margebot_one_review.push_message('!reviews')