On Fri, 02 Sep 2016 16:01:23 +0100 Clement Verna <clement@xxxxxxxxxxxx> wrote: > On 2016-09-02 14:43, Ralph Bean wrote: > > On Fri, Sep 02, 2016 at 10:41:37PM +1000, Will Thames wrote: > >> > >> 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 > > > > +100 to the concept from Will here. > > > > Will, just by way of explanation, we're currently in an > > infrastructure freeze for the Fedora release cycle, which means > > that we want to minimize changes to the infra. Justin sent this to > > the list because (by policy) we require two +1s from members of the > > sysadmin-main group before changes can be applied during freeze. > > > > We want any changes to be as simple as possible (so we can be sure > > it won't explode the world) and easily revertable. Your suggestions > > are great.. but let's wait until after the freeze is up to refactor > > that stuff. :) > > > Sorry if I bring confusion, but I though that the freeze was over. I > believe Kevin sent an email about this earlier this week. We are not in freeze. ;) We left it on wed, the day after the alpha release went out. That said, jflory is in the apprentice group, which doesn't have commit rights, so it would need to be commited by someone and pushed out, so it should still have been posted to the list. ;) So, if someone wants to do the refactor now thats fine too. kevin
Attachment:
pgp7puZ_Dm8qp.pgp
Description: OpenPGP digital signature
_______________________________________________ infrastructure mailing list infrastructure@xxxxxxxxxxxxxxxxxxxxxxx https://lists.fedoraproject.org/admin/lists/infrastructure@xxxxxxxxxxxxxxxxxxxxxxx
