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.
Clement
_______________________________________________
infrastructure mailing list
infrastructure@xxxxxxxxxxxxxxxxxxxxxxx
https://lists.fedoraproject.org/admin/lists/infrastructure@xxxxxxxxxxxxxxxxxxxxxxx
_______________________________________________
infrastructure mailing list
infrastructure@xxxxxxxxxxxxxxxxxxxxxxx
https://lists.fedoraproject.org/admin/lists/infrastructure@xxxxxxxxxxxxxxxxxxxxxxx
