On 2 Sep 2016, at 18:38, Justin W. Flory <jflory7@xxxxxxxxx> wrote: > Hi Will, > > On 09/02/2016 03:15 AM, Will Thames wrote: >> 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. >> > > You can find the source code for the file I'm patching here: > > > https://infrastructure.fedoraproject.org/cgit/ansible.git/tree/roles/fedmsg/irc/templates/ircbot.py > >> >> 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 >> > > Not sure if this is an issue with my patch or the file as a whole. Let > me know if this clarifies the patch. I’d consider tidying up the file to just: config = dict( irc=[ {% for bot in chatbots %} dict( network=‘{{ bot.network|default(“chat.freenode.net”) }}', port={{ bot.port|default(6667) }}, make_pretty={{ bot.pretty|default(True)|bool }}, make_terse={{ bot.terse|default(True)|bool }}, nickname=‘{{ bot.nickname[env] if env in bot.nickname else bot.nickname[‘default’] }}', channel=‘{{ bot_channel }}', filters=dict( topic=[ {{ filters.topic|join(‘,’) }} ], body=[ {{ filters.body|join(,) }} ], ), ), {% endfor %} ] ) And then just have vars/main.yml define the bots. chatbots: - nickname: default: fedmsg-apps staging: fedmsg-apps-s channel: fedora-apps etc Anyway, this is just a suggestion and not intended to be a blocker to implementation >> On Fri, Sep 2, 2016 at 1:14 PM, Justin W. Flory <jflory7@xxxxxxxxx >> <mailto:jflory7@xxxxxxxxx>> 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@xxxxxx <mailto:git@xxxxxx>> >> 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 <http://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@xxxxxxxxx <mailto:jflory7@xxxxxxxxx> >> >> _______________________________________________ >> infrastructure mailing list >> infrastructure@xxxxxxxxxxxxxxxxxxxxxxx >> <mailto:infrastructure@xxxxxxxxxxxxxxxxxxxxxxx> >> https://lists.fedoraproject.org/admin/lists/infrastructure@xxxxxxxxxxxxxxxxxxxxxxx >> <https://lists.fedoraproject.org/admin/lists/infrastructure@xxxxxxxxxxxxxxxxxxxxxxx> >> >> >> >> >> _______________________________________________ >> infrastructure mailing list >> infrastructure@xxxxxxxxxxxxxxxxxxxxxxx >> https://lists.fedoraproject.org/admin/lists/infrastructure@xxxxxxxxxxxxxxxxxxxxxxx >> > > -- > Cheers, > Justin W. Flory > jflory7@xxxxxxxxx > > _______________________________________________ > infrastructure mailing list > infrastructure@xxxxxxxxxxxxxxxxxxxxxxx > https://lists.fedoraproject.org/admin/lists/infrastructure@xxxxxxxxxxxxxxxxxxxxxxx _______________________________________________ infrastructure mailing list infrastructure@xxxxxxxxxxxxxxxxxxxxxxx https://lists.fedoraproject.org/admin/lists/infrastructure@xxxxxxxxxxxxxxxxxxxxxxx
