I hope the below code review is ok. I have a lot of experience with Ansible, but not much experience with Fedora code reviews. I'm just reviewing what I can see from the patch - if anyone can point me at where I can actually see the repo, that would be helpful.


This looks like there could be a for loop in the template, rather than pretty much hardcoding the bot specification for each list.

The code pattern {% if env == "staging" %} is a bit of a red flag (mostly because it leads to problems later where you add a new non-prod env). Really you should be using group variables. So you might have a default bot_suffix of "", and then the staging group (or whichever group is responsible for setting env to staging) would have a bot_suffix of "-s"

Will

On Fri, Sep 2, 2016 at 1:14 PM, Justin W. Flory <jflory7@gmail.com> wrote:
Hi all,

This is my first time submitting a patch to a repo or during a freeze
break - if I missed a step or did something wrong, feel free to correct
me. :) I didn't see a SOP in infra-docs so I just followed the format of
past freeze break requests.

This is a quick patch to add a new bot to ircbot.py for the
#fedora-diversity team. I copied the commopswatch bot and made slight
modifications. But I would appreciate it were reviewed for accuracy in
case I missed something.

Patch is below and also attached to the email.



From 1b182e380e10d978c4daa7f01d859916d5675b78 Mon Sep 17 00:00:00 2001
From: "Justin W. Flory" <git@jwf.io>
Date: Thu, 1 Sep 2016 23:07:04 -0400
Subject: [PATCH] Add #fedora-diversity to ircbot.py for notifications
related
 to diversity

---
 roles/fedmsg/irc/templates/ircbot.py | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/roles/fedmsg/irc/templates/ircbot.py
b/roles/fedmsg/irc/templates/ircbot.py
index 2bb4c03..3f3931b 100644
--- a/roles/fedmsg/irc/templates/ircbot.py
+++ b/roles/fedmsg/irc/templates/ircbot.py
@@ -363,6 +363,27 @@ config = dict(
                 body=['^((?!(modularity|Modularity)).)*$'],
             ),
         ),
+
+        # And #fedora-diversity wanted in too
+        dict(
+            network='chat.freenode.net',
+            port=6667,
+            make_pretty=True,
+            make_terse=True,
+
+            {% if env == 'staging' %}
+            nickname='diversity-bot-s',
+            {% else %}
+            nickname='diversity-bot',
+            {% endif %}
+            channel='fedora-diversity',
+            filters=dict(
+                topic=[
+
'(planet|meetbot\.meeting\.item\.help|meetbot\.meeting|fedocal\.meeting\.new|fedocal\.meeting\.update|fedocal\.meeting\.delete|fedocal\.calendar|fas\.group\.member\.sponsor|pagure\.project\.new|askbot\.post\.flag_offensive)',
+                ],
+                body=['^((?!diversity).)*$'],
+            ),
+        ),
     ],

     ### Possible colors are ###
--
2.7.4



--
Cheers,
Justin W. Flory
jflory7@gmail.com

_______________________________________________
infrastructure mailing list
infrastructure@lists.fedoraproject.org
https://lists.fedoraproject.org/admin/lists/infrastructure@lists.fedoraproject.org