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. :)
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ infrastructure mailing list infrastructure@xxxxxxxxxxxxxxxxxxxxxxx https://lists.fedoraproject.org/admin/lists/infrastructure@xxxxxxxxxxxxxxxxxxxxxxx
