search

devops-coop / ansible-minecraft

Ansible role for provisioning a vanilla Minecraft server
https://galaxy.ansible.com/devops-coop/minecraft/
Apache License 2.0
57 stars 38 forks source link

Support for setting server.properties #5

Closed markrcote closed 8 years ago

markrcote commented 8 years ago

This PR adds support for setting server.properties entries. The bulk of the work is in the first commit, which refactors library/minecraft_acl.py (which is renamed to minecraft_server_file.py in a later commit) to allow support for any file type (not just the JSON ACL files). It also puts the file stats comparison and update procedures into their own class to make the main function a bit easier to read. I'm not super happy that I had to have the module object passed in, but it seemed necessary because that's where the set_*_if_different() methods live. I am of course open to changing it if you have any other ideas.

Hope this is useful!

This change is Reviewable

benwebber commented 8 years ago

Hey, thanks! This is definitely something I want to add support for.

I'll get back to you in the next day or so with some comments.

benwebber commented 8 years ago

I made some recommendations. Overall, I'm quite happy with the ServerFile/FileStats abstractions.

It would be nice to preserve two distinct names for the types of files we're dealing with:

Unfortunately, Ansible doesn't really allow sharing code between modules. To keep those names we'd have to duplicate a bunch of code.

Since this module is internal to the role the naming is of minor consequence.

markrcote commented 8 years ago

Great review! I respect and appreciate your thoroughness. :)

I think I actually first tried to do what you mentioned, creating a new module and trying to share code between them. Can't quite remember as I started this back in January, but your comment sounds really familiar.

I'll fix up these issues soon. Hope you don't mind that I'll be force pushing updates to the commits... it's annoying that GitHub loses some context, but I like to maintain clean commits through the review process.

markrcote commented 8 years ago

By the way, I'm trying out reviewable.io to address your comments. I haven't used it before, but I've heard good things from coworkers. If you want to try it too, it might be the easiest way to coordinate.

benwebber commented 8 years ago

We use Reviewable at work. Happy to use it here. Solves the "refers to an outdated diff" problem.

Reviewed 10 of 10 files at r1. Review status: all files reviewed at latest revision, 16 unresolved discussions.

_README.md, line 107 [r1] (raw file):_

Previously, markrcote (Mark Côté) wrote… > Actually, nevermind. :) I can just make it pass the `minecraft_server_properties` dict as a one-item list to `ServerProperties`. A little hacky, but it works, and it's just implementation that's hidden from the user. >

Modules support a raw type that considers the value as-is.

https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/basic.py#L1480-L1481

You can do your own validation to support multiple types:

module = AnsibleModule(
    argument_spec=dict(
        values=dict(
            required=True,
            type='raw',
        ),
    ),
)

values_type = type(module.params['values'])

if not values_type in (dict, list):
    module.fail_json(msg="implementation error: unknown type {} requested for values".format(values_type))

The caveat is that it won't parse simple key=value argument strings. That's not relevant for this use case.

_library/minecraft_server_file.py, line 87 [r1] (raw file):_

Previously, markrcote (Mark Côté) wrote… > It didn't seem necessary; the abc module doesn't appear to buy us anything here. From my limited experience, abc is mostly concerned with inspection of objects, which we don't do. I can add it if you like, but it doesn't seem like it'd buy us much, right now. >

It's more self-documenting, but it's not really necessary at this point in time. I doubt Minecraft will add more than these file types in the near future.

Minor point: you could remove NotImplementedError since incomplete implementations will throw an exception.

_library/minecraft_server_file.py, line 136 [r1] (raw file):_

I'm starting to think that this solution is more trouble than it's worth, since we can't really get to a proper solution. ConfigParser seems to buy us only two things: the ability to use a dict-type object, and support for : instead of = as separator.

These are valid points. I also hadn't considered how booleans would be serialized, either.

If we think that the default file will stick with simple foo=bar declarations, and if the intention is that customizations to that file on a given server occur only via this ansible role, then having the user provide that style of entry in their playbook seems like the simplest solution and fairly safe—particularly since most users have no idea what a Java properties file is.

I don't expect Minecraft will depart from foo=bar. Most users won't use colons, but users are always surprising.

I prefer a hash for three reasons:

  1. Ansible can merge hashes from multiple files. It can't merge lists.

  2. It demonstrates that we've put considerable thought into the role interface.

    Asking users to enter a list of key=value pairs is not much different from calling lineinfile with the list.

    Something like this is pretty magical in comparison:

     minecraft_server_properties:
   enable-rcon: true
  3. It's the natural data structure for these key/value pairs. There's symmetry between reading and writing the data.

What if we ditch the dirty configparser hack, but keep dictionary input?

_library/minecraft_server_file.py, line 140 [r1] (raw file):_

                self.newlines.append(line)
            else:
                name, eq, val = line.strip().partition('=')

As you mentioned above, you should strip name and val too.

Comments from Reviewable

markrcote commented 8 years ago

Review status: all files reviewed at latest revision, 16 unresolved discussions.

_README.md, line 107 [r1] (raw file):_

Previously, markrcote (Mark Côté) wrote… > Actually, nevermind. :) I can just make it pass the `minecraft_server_properties` dict as a one-item list to `ServerProperties`. A little hacky, but it works, and it's just implementation that's hidden from the user. >

Done.

_library/minecraft_server_file.py, line 19 [r1] (raw file):_

Previously, benwebber (Ben Webber) wrote… > Nitpick: This format will make diffs easier. > > ``` python > > SERVER_FILE_CHOICES = [ > > 'banned-ips', > > 'banned-players', > > …, > > ] > > ``` > >

Done (and sorted the list).

_library/minecraft_server_file.py, line 59 [r1] (raw file):_

Previously, benwebber (Ben Webber) wrote… > These names are redundant (`FileStats.file_stats_*`). > > Recommend `changed` as a property here… >

Done.

_library/minecraft_server_file.py, line 81 [r1] (raw file):_

Previously, benwebber (Ben Webber) wrote… > …and `fix()` or `update()` as a method name here. >

Done.

_library/minecraft_server_file.py, line 96 [r1] (raw file):_

Previously, benwebber (Ben Webber) wrote… > This should be a property (or abstract property). >

Done.

_library/minecraft_server_file.py, line 102 [r1] (raw file):_

Previously, benwebber (Ben Webber) wrote… > Rename to `as_str()`. >

Done.

_library/minecraft_server_file.py, line 108 [r1] (raw file):_

Previously, benwebber (Ben Webber) wrote… > This should also be a property (`changed`). >

Done.

_library/minecraft_server_file.py, line 136 [r1] (raw file):_

Previously, benwebber (Ben Webber) wrote… > > I'm starting to think that this solution is more trouble than it's worth, since we can't really get to a proper solution. ConfigParser seems to buy us only two things: the ability to use a dict-type object, and support for : instead of = as separator. > > These are valid points. I also hadn't considered how booleans would be serialized, either. > > > If we think that the default file will stick with simple foo=bar declarations, and if the intention is that customizations to that file on a given server occur only via this ansible role, then having the user provide that style of entry in their playbook seems like the simplest solution and fairly safe—particularly since most users have no idea what a Java properties file is. > > I don't expect Minecraft will depart from `foo=bar`. Most users won't use colons, but users are always surprising. > > I prefer a hash for three reasons: > 1. Ansible can [merge hashes](https://docs.ansible.com/ansible/intro_configuration.html#hash-behaviour) from multiple files. It can't merge lists. > 2. It demonstrates that we've put considerable thought into the role interface. > > Asking users to enter a list of `key=value` pairs is not much different from calling `lineinfile` with the list. > > Something like this is pretty magical in comparison: > > ``` yaml > minecraft_server_properties: > enable-rcon: true > ``` > 3. It's the natural data structure for these key/value pairs. There's symmetry between reading and writing the data. > > --- > > What if we ditch the dirty `configparser` hack, but keep dictionary input? > >

This doesn't solve the boolean problem, though, since the dictionary input will translate "enable-rcon: true" to {"enable-rcon": True}. Should I just special-case booleans to output "true" or "false", to match what is typical in server.properties?

E.g. I'd probably have to do something like

if type(value) is types.BooleanType:
   value_str = 'true' if value else 'false'
else:
    value_str = str(value)

to get a string value. That will at least handle the common (maybe all?) cases of int, string, and boolean values.

scripts/ci.sh, line 37 [r1] (raw file):

Previously, benwebber (Ben Webber) wrote… > Agreed. Maybe we can switch the CI script to Ansible as well and loop with `until` or `wait_for`. >

I'll file a follow-up issue.

tasks/main.yml, line 122 [r1] (raw file):

Previously, benwebber (Ben Webber) wrote… > Is it necessary to notify the handler here? >

Nope, doesn't seem to be. Removed.

Comments from Reviewable

markrcote commented 8 years ago

Review status: all files reviewed at latest revision, 16 unresolved discussions.

_README.md, line 107 [r1] (raw file):_

Previously, markrcote (Mark Côté) wrote… > Done. >

Sorry, that "done" was in response to your original comment. Would like your opinion on the dict string conversion first before finishing this part.

Comments from Reviewable

markrcote commented 8 years ago

Review status: all files reviewed at latest revision, 16 unresolved discussions.

_library/minecraft_server_file.py, line 136 [r1] (raw file):_

Previously, markrcote (Mark Côté) wrote… > This doesn't solve the boolean problem, though, since the dictionary input will translate "enable-rcon: true" to {"enable-rcon": True}. Should I just special-case booleans to output "true" or "false", to match what is typical in server.properties? > > E.g. I'd probably have to do something like > > ``` > if type(value) is types.BooleanType: > value_str = 'true' if value else 'false' > else: > value_str = str(value) > ``` > > to get a string value. That will at least handle the common (maybe all?) cases of int, string, and boolean values. >

(Or of course the simpler type(value) is bool, but the overall question still stands. :)

Comments from Reviewable

benwebber commented 8 years ago

Review status: all files reviewed at latest revision, 7 unresolved discussions.

_library/minecraft_server_file.py, line 136 [r1] (raw file):_

Previously, markrcote (Mark Côté) wrote… > (Or of course the simpler `type(value) is bool`, but the overall question still stands. :) >

This sounds good. How about:

value_str = str(value).lower() if type(value) is types.BooleanType else str(value)

We can test this, but I think the server would throw an error on startup if users enter collection types. It probably isn't necessary to any additional type validation.

Comments from Reviewable

markrcote commented 8 years ago

Might be easier to switch to reviewing individual commits for this PR.

Review status: 0 of 10 files reviewed at latest revision, 15 unresolved discussions.

Comments from Reviewable

benwebber commented 8 years ago

Reviewed 7 of 10 files at r2, 1 of 1 files at r3, 3 of 3 files at r4, 1 of 1 files at r5, 5 of 5 files at r6, 3 of 3 files at r7. Review status: all files reviewed at latest revision, 12 unresolved discussions.

Comments from Reviewable

benwebber commented 8 years ago

Mark, thank you very much for your hard work and putting up with my nagging. I'm happy to merge this now.

markrcote commented 8 years ago

No problem. Thanks for writing this! :)