This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/203/

config/roles/domaincontroller/role.py (Diff revision 2)
249
        result = yield async.subprocess_future(ipa_install_args)
260
        # result = yield async.subprocess_future(ipa_install_args)
261
        #
262
        # if result.status:
263
        #     # If the subprocess returned non-zero, raise an exception
264
        #     raise RolekitError(COMMAND_FAILED, "%d" % result.status)

Do you want to resurrect this code later? If not, these lines should just go away.


config/roles/domaincontroller/role.py (Diff revision 2)
251
        if result.status:
266
        raise NotImplementedError
252
            # If the subprocess returned non-zero, raise an exception
253
            raise RolekitError(COMMAND_FAILED, "%d" % result.status)
254
267
255
        # Create the systemd target definition
268
        # Create the systemd target definition
256
        target = {'Role': 'domaincontroller',
269
        target = {'Role': 'domaincontroller',
257
                  'Instance': self.get_name(),
270
                  'Instance': self.get_name(),
258
                  'Description': "Domain Controller Role - %s" %
271
                  'Description': "Domain Controller Role - %s" %

The raise in line 266 prevents any later lines from being executed, should they just go away, or is this in the wrong place?


config/roles/domaincontroller/role.py (Diff revision 2)
476
        allowed = re.compile("(?!-)[A-Z\d-]{1,63}(?<!-)$", re.IGNORECASE)
477
        return all(allowed.match(x) for x in fqdn.split("."))

The compiled regex object is used and immediately thrown away here -- which is not a problem, just odd.

Not quoting the regex string with r"..." isn't a problem right now unless someone fiddles with it in the wrong way (i.e. when the backslash becomes a quote for the next character of the string and won't be passed on to the regex compiler)... but it's bad practice ;).

You could just do this instead:

return all(re.match(
    r"(?!-)[A-Z\d-]{1,63}(?<!-)$", x, re.IGNORECASE)
    for x in fqdn.split("."))

Or alternatively compile the regex object once and store it as a class member, of course.


- Nils Philippsen


On September 2nd, 2015, 4:14 nachm. CEST, Stephen Gallagher wrote:

Review request for RoleKit Mailing List, Miloslav Trmac, Nils Philippsen, Stephen Gallagher, and Thomas Woerner.
By Stephen Gallagher.

Updated Sept. 2, 2015, 4:14 nachm.

Bugs: 43
Repository: rolekit

Description

This patch changes the default behavior of the domain controller
role. After discussions with UXD experts, it was determined that
the domain should never be auto-detected from the environment,
since it is very difficult to change later. It should always come
from some user action.

This patch changes the default domain to use the role instance name
for the domain name, as long as the instance name contains valid
characters. It also changes the behavior of the host_name option to
set the hostname to dc-<16 characters> if it is currently set to
localhost.

Fixes https://github.com/libre-server/rolekit/issues/43

Testing

Deployed multiple domain controllers with varying initial hostnames.

Diffs

  • config/roles/domaincontroller/role.py (62d2bc64d6efb9c2b7e254c8d61c612f05e4c4be)
  • doc/xml/rolekit.roles.domaincontroller.xml (2416c889ba0035dc6247b2eb209dbe13eb3b9708)

View Diff