Code review comment for lp:~andreserl/maas/rsyslog_enlistment

Revision history for this message
Mike Pontillo (mpontillo) wrote :

Not getting the port number from /etc/services could be considered a bug, because you should be able to change that, restart your services, and have everything move to the correct port. But based on the config file you pointed me to at debian/extras/20-maas.conf, I agree that's problematic.

If you insist on hard-coding it, then please put it in a common location, such as at the top of compose_preseed.py; don't duplicate it in two places. For example:

# Default port for rsyslog usage in the ephemeral environment
DEFAULT_RSYSLOG_PORT = 514

(I was thinking of suggesting placing it in enum.py, but at least if you do it this way it's consistent with DEFAULT_PORT in maasserver/eventloop.py)

If you don't want to refactor this to have a common function to get the host:port, then I'd be okay with the compromise of grabbing the port number from a common location. But I would just go ahead and put an accessor function in compose_preseed.py to return just the port number in that case anyway, since then none of the other code has to change if/when we refactor it to grab the port number dynamically.

« Back to merge proposal