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 <[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]>
> 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
> [email protected]
>
> _______________________________________________
> infrastructure mailing list
> [email protected]
> https://lists.fedoraproject.org/admin/lists/infrastructure@lists.
> fedoraproject.org
>
>
_______________________________________________
infrastructure mailing list
[email protected]
https://lists.fedoraproject.org/admin/lists/[email protected]
