From ebb794d3429e41434f2d4c53ee3b963695104f04 Mon Sep 17 00:00:00 2001 From: Brad Fults Date: Sat, 27 Feb 2021 17:27:15 -0600 Subject: [PATCH 1/8] Housekeeping & upgrades --- .gitignore | 3 +++ bin/install | 10 ++++++++-- bin/test | 2 +- build-requirements.txt | 12 ++++++------ requirements.txt | 2 +- tests/test_destalinator.py | 24 ++++++++++++------------ 6 files changed, 31 insertions(+), 22 deletions(-) diff --git a/.gitignore b/.gitignore index b9ee905..545e413 100644 --- a/.gitignore +++ b/.gitignore @@ -58,3 +58,6 @@ target/ # Where to store your personal API token *_token.txt + +# VS Code configs +.vscode/ diff --git a/bin/install b/bin/install index 0091a7e..535dd56 100755 --- a/bin/install +++ b/bin/install @@ -1,5 +1,11 @@ #!/bin/sh set -eux -pip install -r build-requirements.txt -pip install -r requirements.txt +if command -v pip3 &> /dev/null +then + pip3 install -r build-requirements.txt + pip3 install -r requirements.txt +else + pip install -r build-requirements.txt + pip install -r requirements.txt +fi diff --git a/bin/test b/bin/test index f018849..0c883c5 100755 --- a/bin/test +++ b/bin/test @@ -6,7 +6,7 @@ flake8 --ignore=E501 # Skip tests/test_destalinator.py due to heavy mocking. See https://github.com/jendrikseipp/vulture/issues/95 # Skip utils/slack_logging.py due to implementing Handler#emit but never calling directly # Skip scheduler.py due to unused lambda entrypoint handler -vulture . --exclude=tests/test_destalinator.py,utils/slack_logging.py,scheduler.py +vulture . --ignore-names=destalinate_lambda coverage run --branch --source=. -m unittest discover -f coverage report -m --skip-covered --fail-under=60 diff --git a/build-requirements.txt b/build-requirements.txt index e4afa46..ac6fe0e 100644 --- a/build-requirements.txt +++ b/build-requirements.txt @@ -1,6 +1,6 @@ -mock==2.0.0 -six==1.10.0 -coverage==4.4.1 -coveralls==1.2.0 -flake8==3.4.1 -vulture==0.26 +coverage>=5.4 +coveralls>=3.0.0 +flake8>=3.8.4 +mock>=4.0.3 +six==1.15.0 +vulture>=2.3 diff --git a/requirements.txt b/requirements.txt index 8a8f286..424973c 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,4 +1,4 @@ APScheduler>=3.0.5 raven>=6.1.0 requests>=2.20.0 -ruamel.yaml \ No newline at end of file +ruamel.yaml>=0.6.12 diff --git a/tests/test_destalinator.py b/tests/test_destalinator.py index 353a9f1..8a0ccaa 100644 --- a/tests/test_destalinator.py +++ b/tests/test_destalinator.py @@ -76,18 +76,18 @@ } ] -sample_warning_messages = [ - { - "user": "U023BCDA1", - "text": "This is a channel warning! Put on your helmets!", - "username": "bot", - "bot_id": "B0T8EDVLY", - "attachments": [{"fallback": "channel_warning", "id": 1}], - "type": "message", - "subtype": "bot_message", - "ts": "1496855882.185855" - } -] +# sample_warning_messages = [ +# { +# "user": "U023BCDA1", +# "text": "This is a channel warning! Put on your helmets!", +# "username": "bot", +# "bot_id": "B0T8EDVLY", +# "attachments": [{"fallback": "channel_warning", "id": 1}], +# "type": "message", +# "subtype": "bot_message", +# "ts": "1496855882.185855" +# } +# ] class MockValidator(object): From eba1e0efc2900b1444532e82db3f4d647ac57e07 Mon Sep 17 00:00:00 2001 From: Brad Fults Date: Sat, 27 Feb 2021 17:27:48 -0600 Subject: [PATCH 2/8] Linter fixes --- destalinator.py | 116 +++++++++++++++++++++++++++++++----------------- flagger.py | 8 ++-- scheduler.py | 2 +- 3 files changed, 81 insertions(+), 45 deletions(-) diff --git a/destalinator.py b/destalinator.py index 4d8890f..c516ebb 100755 --- a/destalinator.py +++ b/destalinator.py @@ -21,12 +21,15 @@ class Destalinator(WithLogger, WithConfig): def __init__(self, slacker, slackbot, activated): """ - slacker is a Slacker() object - slackbot should be an initialized slackbot.Slackbot() object - activated is a boolean indicating whether destalinator should do dry runs or real runs + `slacker` is a Slacker() object + `slackbot` should be an initialized slackbot.Slackbot() object + `activated` is a boolean indicating whether destalinator should do dry + runs or real runs """ - self.closure_text = utils.get_local_file_content(self.closure_text_fname) - self.warning_text = utils.get_local_file_content(self.warning_text_fname) + self.closure_text = utils.get_local_file_content( + self.closure_text_fname) + self.warning_text = utils.get_local_file_content( + self.warning_text_fname) self.slacker = slacker self.slackbot = slackbot @@ -48,11 +51,13 @@ def add_slack_channel_markup_item(self, item): return self.slacker.add_channel_markup(item.group(1)) def add_slack_channel_markup(self, text): - marked_up = re.sub(r"\#([a-z0-9_-]+)", self.add_slack_channel_markup_item, text) + marked_up = re.sub(r"\#([a-z0-9_-]+)", + self.add_slack_channel_markup_item, text) return marked_up def channel_minimum_age(self, channel_name, days): - """Return True if channel represented by `channel_name` is at least `days` old, otherwise False.""" + """Return True if channel represented by `channel_name` is at least + `days` old, otherwise False.""" info = self.slacker.get_channel_info(channel_name) age = info['age'] age = age / 86400 @@ -66,25 +71,31 @@ def flush_channel_cache(self, channel_name): del self.cache[cid] def get_earliest_archive_date(self): - """Return a datetime.date object representing the earliest archive date.""" + """Return a datetime.date object representing the earliest + archive date.""" date_string = self.config.earliest_archive_date \ or PAST_DATE_STRING return datetime.strptime(date_string, "%Y-%m-%d").date() def get_messages(self, channel_name, days): - """Return `days` worth of messages for channel `channel_name`. Caches messages per channel & days.""" + """Return `days` worth of messages for channel `channel_name`. Caches + messages per channel & days.""" oldest = self.now - days * 86400 cid = self.slacker.get_channelid(channel_name) if oldest in self.cache.get(cid, {}): - self.logger.debug("Returning %s cached messages for #%s over %s days", len(self.cache[cid][oldest]), channel_name, days) + self.logger.debug("Returning %s cached messages for #%s over %s days", len( + self.cache[cid][oldest]), channel_name, days) return self.cache[cid][oldest] messages = self.slacker.get_messages_in_time_range(oldest, cid) - self.logger.debug("Fetched %s messages for #%s over %s days", len(messages), channel_name, days) + self.logger.debug("Fetched %s messages for #%s over %s days", len( + messages), channel_name, days) - messages = [x for x in messages if x.get("subtype") is None or x.get("subtype") in self.config.included_subtypes] - self.logger.debug("Filtered down to %s messages based on included_subtypes: %s", len(messages), ", ".join(self.config.included_subtypes)) + messages = [x for x in messages if x.get("subtype") is None or x.get( + "subtype") in self.config.included_subtypes] + self.logger.debug("Filtered down to %s messages based on included_subtypes: %s", len( + messages), ", ".join(self.config.included_subtypes)) if cid not in self.cache: self.cache[cid] = {} @@ -93,7 +104,8 @@ def get_messages(self, channel_name, days): return messages def ignore_channel(self, channel_name): - """Return True if `channel_name` is a channel we should ignore based on config settings.""" + """Return True if `channel_name` is a channel we should ignore based on + config settings.""" if channel_name in self.config.ignore_channels: return True for pat in self.config.ignore_channel_patterns: @@ -102,12 +114,14 @@ def ignore_channel(self, channel_name): return False def post_marked_up_message(self, channel_name, message, **kwargs): - self.slacker.post_message(channel_name, self.add_slack_channel_markup(message), **kwargs) + self.slacker.post_message( + channel_name, self.add_slack_channel_markup(message), **kwargs) def stale(self, channel_name, days): """ Return True if channel represented by `channel_name` is stale. - Definition of stale is: no messages in the last `days` which are not from config.ignore_users. + Definition of stale is: no messages in the last `days` which are not + from config.ignore_users. """ if not self.channel_minimum_age(channel_name, days): return False @@ -139,26 +153,34 @@ def archive(self, channel_name): """Archive the given channel name, returning the Slack API response as a JSON string.""" # Might not need to do this since we now do this in `stale` if self.ignore_channel(channel_name): - self.logger.debug("Not archiving #%s because it's in ignore_channels", channel_name) + self.logger.debug( + "Not archiving #%s because it's in ignore_channels", channel_name) return if self.config.activated: - self.logger.debug("Announcing channel closure in #%s", channel_name) - self.post_marked_up_message(channel_name, self.closure_text, message_type='channel_archive') + self.logger.debug( + "Announcing channel closure in #%s", channel_name) + self.post_marked_up_message( + channel_name, self.closure_text, message_type='channel_archive') members = self.slacker.get_channel_member_names(channel_name) - say = "Members at archiving are {}".format(", ".join(sorted(members))) + say = "Members at archiving are {}".format( + ", ".join(sorted(members))) self.logger.debug("Telling channel #%s: %s", channel_name, say) - self.post_marked_up_message(channel_name, say, message_type='channel_archive_members') + self.post_marked_up_message( + channel_name, say, message_type='channel_archive_members') self.action("Archiving channel #{}".format(channel_name)) payload = self.slacker.archive(channel_name) if payload['ok']: - self.logger.debug("Slack API response to archive: %s", json.dumps(payload, indent=4)) + self.logger.debug( + "Slack API response to archive: %s", json.dumps(payload, indent=4)) self.logger.info("Archived %s", channel_name) else: - error = payload.get('error', '!! No error found in payload %s !!' % payload) - self.logger.error("Failed to archive %s: %s. See https://api.slack.com/methods/conversations.archive for more context.", channel_name, error) + error = payload.get( + 'error', '!! No error found in payload %s !!' % payload) + self.logger.error( + "Failed to archive %s: %s. See https://api.slack.com/methods/conversations.archive for more context.", channel_name, error) return payload @@ -171,18 +193,21 @@ def safe_archive(self, channel_name): # Might not need to do this since we now do this in `stale` if self.slacker.channel_has_only_restricted_members(channel_name): - self.logger.debug("Would have archived #%s but it contains only restricted users", channel_name) + self.logger.debug( + "Would have archived #%s but it contains only restricted users", channel_name) return today = date.today() if today >= self.earliest_archive_date: self.archive(channel_name) else: - self.logger.debug("Would have archived #%s but it's not yet %s", channel_name, self.earliest_archive_date) + self.logger.debug("Would have archived #%s but it's not yet %s", + channel_name, self.earliest_archive_date) def safe_archive_all(self, days): # TODO: No need to pass in days here """Safe archive all channels stale longer than `days`.""" - self.action("Safe-archiving all channels stale for more than {} days".format(days)) + self.action( + "Safe-archiving all channels stale for more than {} days".format(days)) for channel in sorted(self.slacker.channels_by_name.keys()): if self.stale(channel, days): self.logger.debug("Attempting to safe-archive #%s", channel) @@ -197,39 +222,46 @@ def warn(self, channel_name, days, force_warn=False): """ # Might not need to do this since we now do this in `stale` if self.slacker.channel_has_only_restricted_members(channel_name): - self.logger.debug("Would have warned #%s but it contains only restricted users", channel_name) + self.logger.debug( + "Would have warned #%s but it contains only restricted users", channel_name) return False # Might not need to do this since we now do this in `stale` if self.ignore_channel(channel_name): - self.logger.debug("Not warning #%s because it's in ignore_channels", channel_name) + self.logger.debug( + "Not warning #%s because it's in ignore_channels", channel_name) return False messages = self.get_messages(channel_name, days) texts = [x.get("text").strip() for x in messages if x.get("text")] - if (not force_warn and - (self.add_slack_channel_markup(self.warning_text) in texts or - any(any(a.get('fallback') == 'channel_warning' for a in m.get('attachments', [])) for m in messages))): - self.logger.debug("Not warning #%s because we found a prior warning", channel_name) + warned_in_message = self.add_slack_channel_markup(self.warning_text) in texts + warned_in_attachment = any(any(a.get('fallback') == 'channel_warning' for a in m.get('attachments', [])) for m in messages) + if (not force_warn and (warned_in_message or warned_in_attachment)): + self.logger.debug( + "Not warning #%s because we found a prior warning", channel_name) return False if self.config.activated: - self.post_marked_up_message(channel_name, self.warning_text, message_type='channel_warning') + self.post_marked_up_message( + channel_name, self.warning_text, message_type='channel_warning') self.action("Warned #{}".format(channel_name)) return True def warn_all(self, days, force_warn=False): - """Warn all channels which are `days` idle; if `force_warn`, will warn even if we already have.""" + """Warn all channels which are `days` idle; if `force_warn`, will warn + even if we already have.""" if not self.config.activated: self.logger.info("Note, destalinator is not activated and is in a dry-run mode. For help, see the " "documentation on the DESTALINATOR_ACTIVATED environment variable.") - self.action("Warning all channels stale for more than {} days".format(days)) + self.action( + "Warning all channels stale for more than {} days".format(days)) stale = [] for channel in sorted(self.slacker.channels_by_name.keys()): if self.ignore_channel(channel): - self.logger.debug("Not warning #%s because it's in ignore_channels", channel) + self.logger.debug( + "Not warning #%s because it's in ignore_channels", channel) continue if self.stale(channel, days): if self.warn(channel, days, force_warn): @@ -237,7 +269,8 @@ def warn_all(self, days, force_warn=False): self.flush_channel_cache(channel) if stale and self.config.general_message_channel: - self.logger.debug("Notifying #%s of warned channels", self.config.general_message_channel) + self.logger.debug("Notifying #%s of warned channels", + self.config.general_message_channel) self.warn_in_general(stale) def warn_in_general(self, stale_channels): @@ -256,5 +289,8 @@ def warn_in_general(self, stale_channels): message += ", ".join(["#" + x for x in stale_channels]) message = message.format(channel, being, there) if self.config.activated: - self.post_marked_up_message(self.config.general_message_channel, message, message_type='warn_in_general') - self.logger.debug("Notified #%s with: %s", self.config.general_message_channel, message) + self.post_marked_up_message( + self.config.general_message_channel, message, + message_type='warn_in_general') + self.logger.debug("Notified #%s with: %s", + self.config.general_message_channel, message) diff --git a/flagger.py b/flagger.py index d8fa629..82ebf35 100755 --- a/flagger.py +++ b/flagger.py @@ -40,8 +40,8 @@ def extract_threshold(self, token): ]", "", tokens[6]) if output_channel_id.find("|") != -1: - cid, cname = output_channel_id.split("|") + cid, _ = output_channel_id.split("|") output_channel_id = cid output_channel_name = self.slacker.replace_id(output_channel_id) control[uuid] = {'threshold': threshold, "comparator": comparator, @@ -209,7 +209,7 @@ def announce_interesting_messages(self): if not self.debug and self.config.activated: # TODO: rename debug to dry run? self.slackbot.say(output_channel["output"], m) else: - self.logger.warning("Attempted to announce in %s because of rule :%s:%s%s, but channel does not exist.".format( + self.logger.warning("Attempted to announce in {} because of rule :{}:{}{}, but channel does not exist.".format( output_channel["output"], output_channel["emoji"], output_channel["comparator"], diff --git a/scheduler.py b/scheduler.py index f17f504..f094683 100644 --- a/scheduler.py +++ b/scheduler.py @@ -22,7 +22,7 @@ def schedule_job(): sched.start() -def destalinate_lambda(event, context): +def destalinate_lambda(_event, _context): destalinate_job() From cfc04ec6b027db46c9859598ca1543e3fcf9605e Mon Sep 17 00:00:00 2001 From: Brad Fults Date: Sat, 27 Feb 2021 17:34:45 -0600 Subject: [PATCH 3/8] Let Travis manage its own pip --- bin/install | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/bin/install b/bin/install index 535dd56..6e6c374 100755 --- a/bin/install +++ b/bin/install @@ -1,11 +1,16 @@ #!/bin/sh set -eux -if command -v pip3 &> /dev/null -then - pip3 install -r build-requirements.txt - pip3 install -r requirements.txt -else +if [ ${TRAVIS:-false} = 'true' ]; then pip install -r build-requirements.txt pip install -r requirements.txt +else + if command -v pip3 &> /dev/null + then + pip3 install -r build-requirements.txt + pip3 install -r requirements.txt + else + pip install -r build-requirements.txt + pip install -r requirements.txt + fi fi From ef82b4b5628e36388065e71ded3792e94b0e9415 Mon Sep 17 00:00:00 2001 From: Brad Fults Date: Sat, 27 Feb 2021 17:41:12 -0600 Subject: [PATCH 4/8] No more python 2.7 -- it's 2021 --- .travis.yml | 1 - build-requirements.txt | 1 - 2 files changed, 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 7ce7337..cdb2170 100644 --- a/.travis.yml +++ b/.travis.yml @@ -6,7 +6,6 @@ services: language: python python: - - 2.7 - 3.6 cache: pip diff --git a/build-requirements.txt b/build-requirements.txt index ac6fe0e..441e487 100644 --- a/build-requirements.txt +++ b/build-requirements.txt @@ -2,5 +2,4 @@ coverage>=5.4 coveralls>=3.0.0 flake8>=3.8.4 mock>=4.0.3 -six==1.15.0 vulture>=2.3 From effe38c3777f000bd49f58f6c77c07fe05b4c7f8 Mon Sep 17 00:00:00 2001 From: Brad Fults Date: Sat, 27 Feb 2021 18:07:41 -0600 Subject: [PATCH 5/8] Increase destalinator test coverage --- destalinator.py | 19 +---- flagger.py | 2 +- tests/test_destalinator.py | 156 ++++++++++++++++++++++++++++++++++++- 3 files changed, 157 insertions(+), 20 deletions(-) diff --git a/destalinator.py b/destalinator.py index c516ebb..8ba24ad 100755 --- a/destalinator.py +++ b/destalinator.py @@ -220,17 +220,6 @@ def warn(self, channel_name, days, force_warn=False): Using `force_warn=True` will warn even if a previous warning exists. Return True if we actually warned, otherwise False. """ - # Might not need to do this since we now do this in `stale` - if self.slacker.channel_has_only_restricted_members(channel_name): - self.logger.debug( - "Would have warned #%s but it contains only restricted users", channel_name) - return False - - # Might not need to do this since we now do this in `stale` - if self.ignore_channel(channel_name): - self.logger.debug( - "Not warning #%s because it's in ignore_channels", channel_name) - return False messages = self.get_messages(channel_name, days) texts = [x.get("text").strip() for x in messages if x.get("text")] @@ -259,23 +248,17 @@ def warn_all(self, days, force_warn=False): stale = [] for channel in sorted(self.slacker.channels_by_name.keys()): - if self.ignore_channel(channel): - self.logger.debug( - "Not warning #%s because it's in ignore_channels", channel) - continue if self.stale(channel, days): if self.warn(channel, days, force_warn): stale.append(channel) self.flush_channel_cache(channel) - if stale and self.config.general_message_channel: + if len(stale) > 0 and self.config.general_message_channel: self.logger.debug("Notifying #%s of warned channels", self.config.general_message_channel) self.warn_in_general(stale) def warn_in_general(self, stale_channels): - if not stale_channels: - return if len(stale_channels) > 1: channel = "channels" being = "are" diff --git a/flagger.py b/flagger.py index 82ebf35..4e3008d 100755 --- a/flagger.py +++ b/flagger.py @@ -26,7 +26,7 @@ class Flagger(executor.Executor): def __init__(self, *args, **kwargs): self.debug = kwargs.pop('debug', False) - super(self.__class__, self).__init__(*args, **kwargs) + super(Flagger, self).__init__(*args, **kwargs) self.now = int(time.time()) def extract_threshold(self, token): diff --git a/tests/test_destalinator.py b/tests/test_destalinator.py index 8a0ccaa..5188040 100644 --- a/tests/test_destalinator.py +++ b/tests/test_destalinator.py @@ -329,6 +329,20 @@ def test_with_all_sample_messages(self, mock_slacker): self.destalinator.get_messages = mock.MagicMock(return_value=sample_slack_messages) self.assertFalse(self.destalinator.stale('stalinists', 30)) + @mock.patch('tests.test_destalinator.SlackerMock') + def test_with_minimum_channel_age_not_met(self, mock_slacker): + self.destalinator = destalinator.Destalinator(mock_slacker, self.slackbot, activated=True) + mock_slacker.get_channel_info.return_value = {'age': 60 * 86400} + self.destalinator.channel_minimum_age = mock.MagicMock(return_value=False) + self.assertFalse(self.destalinator.stale('stalinists', 30)) + + @mock.patch('tests.test_destalinator.SlackerMock') + def test_with_channel_ignored(self, mock_slacker): + self.destalinator = destalinator.Destalinator(mock_slacker, self.slackbot, activated=True) + mock_slacker.get_channel_info.return_value = {'age': 60 * 86400} + self.destalinator.ignore_channel = mock.MagicMock(return_value=True) + self.assertFalse(self.destalinator.stale('stalinists', 30)) + @mock.patch.object(get_config(), 'ignore_users', [m['user'] for m in sample_slack_messages if m.get('user')]) @mock.patch('tests.test_destalinator.SlackerMock') def test_with_all_users_ignored(self, mock_slacker): @@ -417,6 +431,14 @@ def test_calls_archive_method(self, mock_slacker): self.destalinator.archive("stalinists") mock_slacker.archive.assert_called_once_with('stalinists') + @mock.patch('tests.test_destalinator.SlackerMock') + def test_handles_a_bad_archive_api_response(self, mock_slacker): + self.destalinator = destalinator.Destalinator(mock_slacker, self.slackbot, activated=True) + mock_slacker.post_message.return_value = {} + mock_slacker.archive.return_value = {'ok': False, 'error': 'yup'} + self.destalinator.archive("stalinists") + mock_slacker.archive.assert_called_once_with('stalinists') + class DestalinatorSafeArchiveTestCase(unittest.TestCase): def setUp(self): @@ -459,9 +481,22 @@ def setUp(self): self.slackbot = slackbot.Slackbot("testing", "token") @mock.patch('tests.test_destalinator.SlackerMock') - def test_calls_stale_once_for_each_channel(self, mock_slacker): + def test_calls_stale_once_for_each_channel_with_cache(self, mock_slacker): self.destalinator = destalinator.Destalinator(mock_slacker, self.slackbot, activated=True) mock_slacker.channels_by_name = {'leninists': 'C012839', 'stalinists': 'C102843'} + mock_slacker.get_channelid.return_value = 'C012839' + days_ago = self.destalinator.now - (86400 * 30) + self.destalinator.cache = {'C012839': {days_ago: sample_slack_messages}} + self.destalinator.stale = mock.MagicMock(return_value=False) + days = self.destalinator.config.archive_threshold + self.destalinator.safe_archive_all(days) + self.assertEqual(self.destalinator.stale.mock_calls, [mock.call('leninists', days), mock.call('stalinists', days)]) + + @mock.patch('tests.test_destalinator.SlackerMock') + def test_calls_stale_once_for_each_channel_without_cache(self, mock_slacker): + self.destalinator = destalinator.Destalinator(mock_slacker, self.slackbot, activated=True) + mock_slacker.channels_by_name = {'leninists': 'C012839', 'stalinists': 'C102843'} + mock_slacker.get_channelid.return_value = 'C012839' self.destalinator.stale = mock.MagicMock(return_value=False) days = self.destalinator.config.archive_threshold self.destalinator.safe_archive_all(days) @@ -512,6 +547,18 @@ def test_warns_by_posting_message(self, mock_slacker): self.destalinator.warning_text, message_type='channel_warning') + @mock.patch('tests.test_destalinator.SlackerMock') + def test_warns_with_cached_messages(self, mock_slacker): + self.destalinator = destalinator.Destalinator(mock_slacker, self.slackbot, activated=True) + days_ago = self.destalinator.now - (86400 * 30) + self.destalinator.cache = {'C012839': {days_ago: sample_slack_messages}} + mock_slacker.get_channelid.return_value = 'C012839' + mock_slacker.channel_has_only_restricted_members.return_value = False + self.destalinator.warn("stalinists", 30) + mock_slacker.post_message.assert_called_with("stalinists", + self.destalinator.warning_text, + message_type='channel_warning') + def test_warns_by_posting_message_with_channel_names(self): self.destalinator = destalinator.Destalinator(self.slacker, self.slackbot, activated=True) warning_text = self.destalinator.warning_text + " #leninists" @@ -552,3 +599,110 @@ def test_does_not_warn_when_previous_warning_with_changed_text_found(self, mock_ ] self.destalinator.warn("stalinists", 30) self.assertFalse(mock_slacker.post_message.called) + + +class DestalinatorWarnAllTestCase(unittest.TestCase): + def setUp(self): + self.slacker = SlackerMock("testing", "token") + self.slackbot = slackbot.Slackbot("testing", "token") + + @mock.patch('tests.test_destalinator.SlackerMock') + def test_posts_a_warning_for_a_stale_channel(self, mock_slacker): + self.destalinator = destalinator.Destalinator(mock_slacker, self.slackbot, activated=True) + mock_slacker.channels_by_name = {'leninists': 'C012839', 'stalinists': 'C102843'} + mock_slacker.channel_has_only_restricted_members.return_value = False + + def fake_add_channel_markup(channel: str): + return "<#{}|C012839>".format(channel) + + mock_slacker.add_channel_markup = mock.MagicMock(side_effect=fake_add_channel_markup) + + def fake_stale(channel, days): + return {'leninists': True, 'stalinists': False}[channel] + + self.destalinator.stale = mock.MagicMock(side_effect=fake_stale) + self.destalinator.warn_all(self.destalinator.config.archive_threshold) + self.assertTrue(mock_slacker.post_message.called) + + @mock.patch('tests.test_destalinator.SlackerMock') + def test_posts_no_warning_during_dry_run(self, mock_slacker): + self.destalinator = destalinator.Destalinator(mock_slacker, self.slackbot, activated=False) + mock_slacker.channels_by_name = {'leninists': 'C012839', 'stalinists': 'C102843'} + mock_slacker.channel_has_only_restricted_members.return_value = False + + def fake_stale(channel, days): + return {'leninists': True, 'stalinists': False}[channel] + + self.destalinator.stale = mock.MagicMock(side_effect=fake_stale) + self.destalinator.warn_all(self.destalinator.config.archive_threshold) + self.assertFalse(mock_slacker.post_message.called) + + @mock.patch.object(get_config(), 'general_message_channel', False) + @mock.patch('tests.test_destalinator.SlackerMock') + def test_does_not_warn_without_general_channel_configured(self, mock_slacker): + self.destalinator = destalinator.Destalinator(mock_slacker, self.slackbot, activated=True) + mock_slacker.channels_by_name = {'leninists': 'C012839', 'stalinists': 'C102843'} + mock_slacker.channel_has_only_restricted_members.return_value = False + + def fake_stale(channel, days): + return {'leninists': True, 'stalinists': False}[channel] + + self.destalinator.stale = mock.MagicMock(side_effect=fake_stale) + self.destalinator.warn_in_general = mock.MagicMock() + + self.destalinator.warn_all(self.destalinator.config.archive_threshold) + self.assertFalse(self.destalinator.warn_in_general.called) + + @mock.patch.object(get_config(), 'general_message_channel', 'stalinists') + @mock.patch('tests.test_destalinator.SlackerMock') + def test_does_not_warn_in_general_channel_with_no_stale_channels(self, mock_slacker): + self.destalinator = destalinator.Destalinator(mock_slacker, self.slackbot, activated=True) + mock_slacker.channels_by_name = {'leninists': 'C012839', 'stalinists': 'C102843'} + mock_slacker.channel_has_only_restricted_members.return_value = False + + def fake_stale(channel, days): + return False + + self.destalinator.stale = mock.MagicMock(side_effect=fake_stale) + self.destalinator.warn_in_general = mock.MagicMock(return_value=True) + + self.destalinator.warn_all(self.destalinator.config.archive_threshold) + self.assertFalse(self.destalinator.warn_in_general.called) + + @mock.patch.object(get_config(), 'general_message_channel', 'stalinists') + @mock.patch('tests.test_destalinator.SlackerMock') + def test_warns_in_general_channel_with_one_stale_channel(self, mock_slacker): + self.destalinator = destalinator.Destalinator(mock_slacker, self.slackbot, activated=True) + mock_slacker.channels_by_name = {'leninists': 'C012839', 'stalinists': 'C102843'} + mock_slacker.channel_has_only_restricted_members.return_value = False + + def fake_add_channel_markup(channel: str): + return "<#{}|C012839>".format(channel) + + mock_slacker.add_channel_markup = mock.MagicMock(side_effect=fake_add_channel_markup) + + def fake_stale(channel, days): + return {'leninists': True, 'stalinists': False}[channel] + + self.destalinator.stale = mock.MagicMock(side_effect=fake_stale) + self.destalinator.warn_all(self.destalinator.config.archive_threshold) + self.assertTrue(mock_slacker.post_message.called) + + @mock.patch.object(get_config(), 'general_message_channel', 'stalinists') + @mock.patch('tests.test_destalinator.SlackerMock') + def test_warns_in_general_channel_with_two_stale_channels(self, mock_slacker): + self.destalinator = destalinator.Destalinator(mock_slacker, self.slackbot, activated=True) + mock_slacker.channels_by_name = {'leninists': 'C012839', 'stalinists': 'C102843'} + mock_slacker.channel_has_only_restricted_members.return_value = False + + def fake_add_channel_markup(channel: str): + return "<#{}|C012839>".format(channel) + + mock_slacker.add_channel_markup = mock.MagicMock(side_effect=fake_add_channel_markup) + + def fake_stale(channel, days): + return True + + self.destalinator.stale = mock.MagicMock(side_effect=fake_stale) + self.destalinator.warn_all(self.destalinator.config.archive_threshold) + self.assertTrue(mock_slacker.post_message.called) From ca01c29ff55bea1601460a8baeaa199089b8108b Mon Sep 17 00:00:00 2001 From: Brad Fults Date: Fri, 12 Mar 2021 21:00:26 -0600 Subject: [PATCH 6/8] Embraces 120-column line length --- .config/pycodestyle | 2 + destalinator.py | 89 ++++++++++++++++----------------------------- scheduler.py | 6 +-- 3 files changed, 35 insertions(+), 62 deletions(-) create mode 100644 .config/pycodestyle diff --git a/.config/pycodestyle b/.config/pycodestyle new file mode 100644 index 0000000..9d54e0f --- /dev/null +++ b/.config/pycodestyle @@ -0,0 +1,2 @@ +[pycodestyle] +max_line_length = 120 diff --git a/destalinator.py b/destalinator.py index 8ba24ad..9f71c77 100755 --- a/destalinator.py +++ b/destalinator.py @@ -26,10 +26,8 @@ def __init__(self, slacker, slackbot, activated): `activated` is a boolean indicating whether destalinator should do dry runs or real runs """ - self.closure_text = utils.get_local_file_content( - self.closure_text_fname) - self.warning_text = utils.get_local_file_content( - self.warning_text_fname) + self.closure_text = utils.get_local_file_content( self.closure_text_fname) + self.warning_text = utils.get_local_file_content( self.warning_text_fname) self.slacker = slacker self.slackbot = slackbot @@ -51,13 +49,11 @@ def add_slack_channel_markup_item(self, item): return self.slacker.add_channel_markup(item.group(1)) def add_slack_channel_markup(self, text): - marked_up = re.sub(r"\#([a-z0-9_-]+)", - self.add_slack_channel_markup_item, text) + marked_up = re.sub(r"\#([a-z0-9_-]+)", self.add_slack_channel_markup_item, text) return marked_up def channel_minimum_age(self, channel_name, days): - """Return True if channel represented by `channel_name` is at least - `days` old, otherwise False.""" + """Return True if channel represented by `channel_name` is at least `days` old, otherwise False.""" info = self.slacker.get_channel_info(channel_name) age = info['age'] age = age / 86400 @@ -71,15 +67,12 @@ def flush_channel_cache(self, channel_name): del self.cache[cid] def get_earliest_archive_date(self): - """Return a datetime.date object representing the earliest - archive date.""" - date_string = self.config.earliest_archive_date \ - or PAST_DATE_STRING + """Return a datetime.date object representing the earliest archive date.""" + date_string = self.config.earliest_archive_date or PAST_DATE_STRING return datetime.strptime(date_string, "%Y-%m-%d").date() def get_messages(self, channel_name, days): - """Return `days` worth of messages for channel `channel_name`. Caches - messages per channel & days.""" + """Return `days` worth of messages for channel `channel_name`. Caches messages per channel & days.""" oldest = self.now - days * 86400 cid = self.slacker.get_channelid(channel_name) @@ -89,8 +82,7 @@ def get_messages(self, channel_name, days): return self.cache[cid][oldest] messages = self.slacker.get_messages_in_time_range(oldest, cid) - self.logger.debug("Fetched %s messages for #%s over %s days", len( - messages), channel_name, days) + self.logger.debug("Fetched %s messages for #%s over %s days", len(messages), channel_name, days) messages = [x for x in messages if x.get("subtype") is None or x.get( "subtype") in self.config.included_subtypes] @@ -104,8 +96,7 @@ def get_messages(self, channel_name, days): return messages def ignore_channel(self, channel_name): - """Return True if `channel_name` is a channel we should ignore based on - config settings.""" + """Return True if `channel_name` is a channel we should ignore based on config settings.""" if channel_name in self.config.ignore_channels: return True for pat in self.config.ignore_channel_patterns: @@ -114,8 +105,7 @@ def ignore_channel(self, channel_name): return False def post_marked_up_message(self, channel_name, message, **kwargs): - self.slacker.post_message( - channel_name, self.add_slack_channel_markup(message), **kwargs) + self.slacker.post_message( channel_name, self.add_slack_channel_markup(message), **kwargs) def stale(self, channel_name, days): """ @@ -153,34 +143,27 @@ def archive(self, channel_name): """Archive the given channel name, returning the Slack API response as a JSON string.""" # Might not need to do this since we now do this in `stale` if self.ignore_channel(channel_name): - self.logger.debug( - "Not archiving #%s because it's in ignore_channels", channel_name) + self.logger.debug( "Not archiving #%s because it's in ignore_channels", channel_name) return if self.config.activated: - self.logger.debug( - "Announcing channel closure in #%s", channel_name) - self.post_marked_up_message( - channel_name, self.closure_text, message_type='channel_archive') + self.logger.debug("Announcing channel closure in #%s", channel_name) + self.post_marked_up_message(channel_name, self.closure_text, message_type='channel_archive') members = self.slacker.get_channel_member_names(channel_name) - say = "Members at archiving are {}".format( - ", ".join(sorted(members))) + say = "Members at archiving are {}".format(", ".join(sorted(members))) self.logger.debug("Telling channel #%s: %s", channel_name, say) - self.post_marked_up_message( - channel_name, say, message_type='channel_archive_members') + self.post_marked_up_message(channel_name, say, message_type='channel_archive_members') self.action("Archiving channel #{}".format(channel_name)) payload = self.slacker.archive(channel_name) if payload['ok']: - self.logger.debug( - "Slack API response to archive: %s", json.dumps(payload, indent=4)) + self.logger.debug("Slack API response to archive: %s", json.dumps(payload, indent=4)) self.logger.info("Archived %s", channel_name) else: - error = payload.get( - 'error', '!! No error found in payload %s !!' % payload) - self.logger.error( - "Failed to archive %s: %s. See https://api.slack.com/methods/conversations.archive for more context.", channel_name, error) + error = payload.get('error', '!! No error found in payload %s !!' % payload) + url = "https://api.slack.com/methods/conversations.archive" + self.logger.error("Failed to archive %s: %s. See "+ url +" for more context.", channel_name, error) return payload @@ -193,21 +176,18 @@ def safe_archive(self, channel_name): # Might not need to do this since we now do this in `stale` if self.slacker.channel_has_only_restricted_members(channel_name): - self.logger.debug( - "Would have archived #%s but it contains only restricted users", channel_name) + self.logger.debug("Would have archived #%s but it contains only restricted users", channel_name) return today = date.today() if today >= self.earliest_archive_date: self.archive(channel_name) else: - self.logger.debug("Would have archived #%s but it's not yet %s", - channel_name, self.earliest_archive_date) + self.logger.debug("Would have archived #%s but it's not yet %s", channel_name, self.earliest_archive_date) def safe_archive_all(self, days): # TODO: No need to pass in days here """Safe archive all channels stale longer than `days`.""" - self.action( - "Safe-archiving all channels stale for more than {} days".format(days)) + self.action("Safe-archiving all channels stale for more than {} days".format(days)) for channel in sorted(self.slacker.channels_by_name.keys()): if self.stale(channel, days): self.logger.debug("Attempting to safe-archive #%s", channel) @@ -224,27 +204,24 @@ def warn(self, channel_name, days, force_warn=False): messages = self.get_messages(channel_name, days) texts = [x.get("text").strip() for x in messages if x.get("text")] warned_in_message = self.add_slack_channel_markup(self.warning_text) in texts - warned_in_attachment = any(any(a.get('fallback') == 'channel_warning' for a in m.get('attachments', [])) for m in messages) + warned_in_attachment = any(any(a.get('fallback') == 'channel_warning' for a in m.get( + 'attachments', [])) for m in messages) if (not force_warn and (warned_in_message or warned_in_attachment)): - self.logger.debug( - "Not warning #%s because we found a prior warning", channel_name) + self.logger.debug("Not warning #%s because we found a prior warning", channel_name) return False if self.config.activated: - self.post_marked_up_message( - channel_name, self.warning_text, message_type='channel_warning') + self.post_marked_up_message(channel_name, self.warning_text, message_type='channel_warning') self.action("Warned #{}".format(channel_name)) return True def warn_all(self, days, force_warn=False): - """Warn all channels which are `days` idle; if `force_warn`, will warn - even if we already have.""" + """Warn all channels which are `days` idle; if `force_warn`, will warn even if we already have.""" if not self.config.activated: self.logger.info("Note, destalinator is not activated and is in a dry-run mode. For help, see the " "documentation on the DESTALINATOR_ACTIVATED environment variable.") - self.action( - "Warning all channels stale for more than {} days".format(days)) + self.action("Warning all channels stale for more than {} days".format(days)) stale = [] for channel in sorted(self.slacker.channels_by_name.keys()): @@ -254,8 +231,7 @@ def warn_all(self, days, force_warn=False): self.flush_channel_cache(channel) if len(stale) > 0 and self.config.general_message_channel: - self.logger.debug("Notifying #%s of warned channels", - self.config.general_message_channel) + self.logger.debug("Notifying #%s of warned channels", self.config.general_message_channel) self.warn_in_general(stale) def warn_in_general(self, stale_channels): @@ -272,8 +248,5 @@ def warn_in_general(self, stale_channels): message += ", ".join(["#" + x for x in stale_channels]) message = message.format(channel, being, there) if self.config.activated: - self.post_marked_up_message( - self.config.general_message_channel, message, - message_type='warn_in_general') - self.logger.debug("Notified #%s with: %s", - self.config.general_message_channel, message) + self.post_marked_up_message( self.config.general_message_channel, message, message_type='warn_in_general') + self.logger.debug("Notified #%s with: %s", self.config.general_message_channel, message) diff --git a/scheduler.py b/scheduler.py index f094683..ed4c141 100644 --- a/scheduler.py +++ b/scheduler.py @@ -31,10 +31,8 @@ def destalinate_job(): logging.info("Destalinating") if not get_config().sb_token or not get_config().api_token: - logging.error( - "Missing at least one required Slack environment variable.\n" - "Make sure to set DESTALINATOR_SB_TOKEN and DESTALINATOR_API_TOKEN." - ) + logging.error("Missing at least one required Slack environment variable.\n" + "Make sure to set DESTALINATOR_SB_TOKEN and DESTALINATOR_API_TOKEN.") else: try: archiver.Archiver().archive() From 0875ba4d18c903e4dbcb3a35f1e41644f1ad3921 Mon Sep 17 00:00:00 2001 From: Brad Fults Date: Fri, 12 Mar 2021 21:03:46 -0600 Subject: [PATCH 7/8] Improve accuracy of code comments --- bin/test | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/bin/test b/bin/test index 0c883c5..866b32c 100755 --- a/bin/test +++ b/bin/test @@ -3,9 +3,7 @@ set -eux flake8 --ignore=E501 -# Skip tests/test_destalinator.py due to heavy mocking. See https://github.com/jendrikseipp/vulture/issues/95 -# Skip utils/slack_logging.py due to implementing Handler#emit but never calling directly -# Skip scheduler.py due to unused lambda entrypoint handler +# Skip uncalled lambda entry point vulture . --ignore-names=destalinate_lambda coverage run --branch --source=. -m unittest discover -f From 5c1baec077e7b8482c0dc77870317ad8958460bd Mon Sep 17 00:00:00 2001 From: Brad Fults Date: Fri, 12 Mar 2021 21:38:25 -0600 Subject: [PATCH 8/8] Remove whitespace errors --- destalinator.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/destalinator.py b/destalinator.py index 9f71c77..2a029d4 100755 --- a/destalinator.py +++ b/destalinator.py @@ -26,8 +26,8 @@ def __init__(self, slacker, slackbot, activated): `activated` is a boolean indicating whether destalinator should do dry runs or real runs """ - self.closure_text = utils.get_local_file_content( self.closure_text_fname) - self.warning_text = utils.get_local_file_content( self.warning_text_fname) + self.closure_text = utils.get_local_file_content(self.closure_text_fname) + self.warning_text = utils.get_local_file_content(self.warning_text_fname) self.slacker = slacker self.slackbot = slackbot @@ -105,7 +105,7 @@ def ignore_channel(self, channel_name): return False def post_marked_up_message(self, channel_name, message, **kwargs): - self.slacker.post_message( channel_name, self.add_slack_channel_markup(message), **kwargs) + self.slacker.post_message(channel_name, self.add_slack_channel_markup(message), **kwargs) def stale(self, channel_name, days): """ @@ -143,7 +143,7 @@ def archive(self, channel_name): """Archive the given channel name, returning the Slack API response as a JSON string.""" # Might not need to do this since we now do this in `stale` if self.ignore_channel(channel_name): - self.logger.debug( "Not archiving #%s because it's in ignore_channels", channel_name) + self.logger.debug("Not archiving #%s because it's in ignore_channels", channel_name) return if self.config.activated: @@ -163,7 +163,7 @@ def archive(self, channel_name): else: error = payload.get('error', '!! No error found in payload %s !!' % payload) url = "https://api.slack.com/methods/conversations.archive" - self.logger.error("Failed to archive %s: %s. See "+ url +" for more context.", channel_name, error) + self.logger.error("Failed to archive %s: %s. See " + url + " for more context.", channel_name, error) return payload @@ -248,5 +248,5 @@ def warn_in_general(self, stale_channels): message += ", ".join(["#" + x for x in stale_channels]) message = message.format(channel, being, there) if self.config.activated: - self.post_marked_up_message( self.config.general_message_channel, message, message_type='warn_in_general') + self.post_marked_up_message(self.config.general_message_channel, message, message_type='warn_in_general') self.logger.debug("Notified #%s with: %s", self.config.general_message_channel, message)