On 2 Sep 2016, at 18:38, Justin W. Flory <[email protected]> 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 <[email protected]
>> <mailto:[email protected]>> 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" <[email protected] <mailto:[email protected]>>
>> 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
>> [email protected] <mailto:[email protected]>
>>
>> _______________________________________________
>> infrastructure mailing list
>> [email protected]
>> <mailto:[email protected]>
>>
>> https://lists.fedoraproject.org/admin/lists/[email protected]
>>
>> <https://lists.fedoraproject.org/admin/lists/[email protected]>
>>
>>
>>
>>
>> _______________________________________________
>> infrastructure mailing list
>> [email protected]
>> https://lists.fedoraproject.org/admin/lists/[email protected]
>>
>
> --
> Cheers,
> Justin W. Flory
> [email protected]
>
> _______________________________________________
> infrastructure mailing list
> [email protected]
> https://lists.fedoraproject.org/admin/lists/[email protected]
_______________________________________________
infrastructure mailing list
[email protected]
https://lists.fedoraproject.org/admin/lists/[email protected]
