On 09/02/2016 04:53 PM, Kevin Fenzi wrote: > 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 > I think it would probably be better for a more experienced Pythonista to refactor too – I'm still hardly a novice and it would probably be more to the point if someone else tackled it. :) -- Cheers, Justin W. Flory jflory7@xxxxxxxxx
Attachment:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ infrastructure mailing list infrastructure@xxxxxxxxxxxxxxxxxxxxxxx https://lists.fedoraproject.org/admin/lists/infrastructure@xxxxxxxxxxxxxxxxxxxxxxx
