On 2 Sep 2016, at 18:38, Justin W. Flory <jflory7(a)gmail.com> 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/fedm...
>
> 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(a)gmail.com
> <mailto: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(a)jwf.io <mailto: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
<
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(a)gmail.com <mailto:jflory7@gmail.com>
>
> _______________________________________________
> infrastructure mailing list
> infrastructure(a)lists.fedoraproject.org
> <mailto:infrastructure@lists.fedoraproject.org>
>
https://lists.fedoraproject.org/admin/lists/infrastructure@lists.fedorapr...
>
<
https://lists.fedoraproject.org/admin/lists/infrastructure@lists.fedorapr...
>
>
>
>
> _______________________________________________
> infrastructure mailing list
> infrastructure(a)lists.fedoraproject.org
>
https://lists.fedoraproject.org/admin/lists/infrastructure@lists.fedorapr...
>
--
Cheers,
Justin W. Flory
jflory7(a)gmail.com
_______________________________________________
infrastructure mailing list
infrastructure(a)lists.fedoraproject.org
https://lists.fedoraproject.org/admin/lists/infrastructure@lists.fedorapr...