Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

osx_defaults: refactor to use Foundation/CoreFoudation python modules instead of defaults command, adding a lot of new features at the same time (nested keys and values, support for all types, etc.) #24808

Closed
wants to merge 1 commit into from

Conversation

etienned
Copy link

@etienned etienned commented May 19, 2017

SUMMARY

Update of the osx_defaults module to use Foundation / CoreFoudation python modules instead of the defaults command. The goal is to add new features and those features were not possible using the defaults command. New features are:

  • Access to nested keys and indexes (ex.: DesktopViewSettings -> IconViewSettings -> arrangeBy).
  • Writing nested and complex values (ex.: [1, {key1: 'text', key2: true}, 'other stuff']).
  • Support for all data types: array, boolean, data, date, dict, float, integer, string.
  • Support to access preferences for any users (like network settings, etc.).
  • Specifying type is now optional. Types are mostly deduce as YAML/Ansible do.

Those changes should be backwards compatible.

Also this update fixes some issues that are present in the current implementation (array_add not keeping items order, etc.).

Code cleanup to adhere to Ansible's guidelines.

Not python 3 compatible for the moment. But in most case Foundation / CoreFoundation modules runs under the stock macOS' python and it's currently version 2.7. It should not be too hard to add support for python 3 after the module will be tested for some time in the wild.

The new version of the module is built in way that it could be relatively easy to add other backends like defaults and PlistBuddy commands. It could be possible to automatically fallback to another backend (ex. using defaults, so module will have with limited functionalities) if Foundation can't be imported. I don't know if it's a good or a bad idea.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

osx_defaults

ANSIBLE VERSION
ansible 2.4.0 (devel da99deeddc) last updated 2017/05/18 22:45:57 (GMT -400)
ADDITIONAL INFORMATION

I know there's already at least one other pull request (like this one) for an updated version of osx_defaults but I think this version is more complete. I started to work on this a long time ago after I submit a fix for a bug. Unfortunately I didn't have a lot of time to work on it. I also did many iterations, first one was using PlistBuddy.

I did a mostly complete test suite for the main class (CFPreferences) but unfortunately at the time I start it there was no official way to test modules. Also it was not possible to import something, like a class, from an Ansible module. So my test suite is built on pytest and the class is simply copied in the test file. That's not really pretty. You can find it here. If there's a relatively simple way to integrate this test suite to Ansible, tell me. I also have a test playbook that cover most combinations here.

I would like also to mention that I had to circumvent some inconsistent Ansible behaviors, particularly on how Ansible cast types in YAML, as this module use YAML structure and types intensively. I know there's some work that is done on that side currently and it's a good thing. I hit some limitations of JSON: no support for datetime and for data. That's one of the reason that datetime should always be quoted for this module. Maybe a kind of support for those types could be added directly to Ansible in the future.

I tried to document the code correctly but if you have any interrogations just ask.

@ansibot ansibot added affects_2.4 This issue/PR affects Ansible v2.4 feature_pull_request module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_triage Needs a first human triage before being processed. python3 labels May 19, 2017
@gundalow gundalow added the ci_verified Changes made in this PR are causing tests to fail. label May 22, 2017
@alikins alikins removed the needs_triage Needs a first human triage before being processed. label May 22, 2017
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jun 15, 2017
@ansibot ansibot added the support:community This issue/PR relates to code supported by the Ansible community. label Jun 29, 2017
@ansibot ansibot added support:core This issue/PR relates to code supported by the Ansible Engineering Team. test_pull_requests and removed ci_verified Changes made in this PR are causing tests to fail. module This issue/PR relates to a module. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 9, 2017
@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. needs_maintainer Ansibot is unable to identify maintainers for this PR. (Check `author` in docs or BOTMETA.yml) stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jul 18, 2017
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Jul 26, 2017
@ansibot ansibot removed the support:core This issue/PR relates to code supported by the Ansible Engineering Team. label Aug 24, 2017
@ansibot ansibot added the test This PR relates to tests. label Sep 1, 2017
@abadger abadger removed the python3 label Sep 9, 2017
@booch
Copy link

booch commented Sep 20, 2017

This rewrite has everything I've dreamed of for the osx_defaults module, and more. There are a lot of situations where I still need to fall back to using the defaults command. This rewrite resolves them all. Very nicely done! I'd really like to see this go into 2.5 ASAP.

@booch
Copy link

booch commented Sep 26, 2017

So what's the process for getting this included in the Ansible 2.5 release? What can we do to help the process along?

@ansibot ansibot added the new_contributor This PR is the first contribution by a new community member. label Oct 18, 2017
@etienned
Copy link
Author

@kinglouie I've checked your tests in your PR and it's pretty clear. Thanks!

@dagwieers
Copy link
Contributor

@etienned Can you check which of the existing open osx_defaults issues your refactor would solve ? We could ask those reporters to test if you branch fixes their issue as well.

https://github.com/ansible/ansible/issues?utf8=%E2%9C%93&q=is%3Aissue+is%3Aopen+osx_defaults

@ansibot
Copy link
Contributor

ansibot commented Feb 3, 2019

@ansibot ansibot removed the deprecated This issue/PR relates to a deprecated module. label Feb 3, 2019
@gundalow gundalow added pep8 Here be dragons and removed pep8 Here be dragons labels Feb 5, 2019
@ansibot ansibot added the support:community This issue/PR relates to code supported by the Ansible community. label Feb 5, 2019
@ansibot ansibot added the system System category label Feb 13, 2019
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Feb 21, 2019
@ansibot
Copy link
Contributor

ansibot commented Mar 1, 2019

@jamesdh
Copy link

jamesdh commented Apr 2, 2019

Just bumped into a need for this today. Please don't let this die like #24028 did :(

@etienned
Copy link
Author

etienned commented Apr 3, 2019

Unfortunately, I'm pretty busy right now but I'm not forgetting it!

@ansibot
Copy link
Contributor

ansibot commented May 29, 2019

@kinglouie
Copy link

I would be really glad if this could get the attention it deserves. I'm really grateful for the effort you put into this @etienned but I think with the limited time you can offer to support this PR it would be beneficial for everyone who wants to use this, if you could approve that someone else finishes this. It's already 1,5 years later after I pushed an adaption of this PR because at the time it was already stale in here.

@dsedivec
Copy link
Contributor

This looks like a big improvement upon the existing osx_defaults module, thanks!

One possible problem: Consider domain com.apple.symbolichotkeys, key AppleSymbolicHotKeys, which is a dict with ints as keys:

{
    AppleSymbolicHotKeys =     {
        32 =         {
            enabled = 1;
            value =             {
                parameters =                 (
                    65535,
                    126,
                    262144
                );
                type = standard;
            };
        };
};

Trying to one of the keys of AppleSymbolicHotKeys with something like key="AppleSymbolicHotKeys:32" results in an error like:

Type mismatch between the key `32` and the node `{...long value...}` (<type 'list'> -> <type 'dict'>).

I believe this module thinks the integer key means it should expect a list, which is not true in this case.

@ansibot ansibot added collection Related to Ansible Collections work collection:community.general labels Apr 29, 2020
@Akasurde Akasurde added the needs_collection_redirect https://github.com/ansible/ansibullbot/blob/master/docs/collection_migration.md label Aug 19, 2020
@ansibot ansibot removed the needs_collection_redirect https://github.com/ansible/ansibullbot/blob/master/docs/collection_migration.md label Aug 19, 2020
@Akasurde
Copy link
Member

Hi @etienned, Thank you very much for your interest in Ansible. This plugin/module is no longer maintained in this repository and has been migrated to https://github.com/ansible-collections/community.general

If you have further questions please stop by IRC or the mailing list:

* IRC: #ansible on irc.freenode.net
* mailing list: https://groups.google.com/forum/#!forum/ansible-project

needs_info

@Akasurde Akasurde closed this Aug 20, 2020
@ansible ansible locked as resolved and limited conversation to collaborators Aug 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.4 This issue/PR affects Ansible v2.4 collection:community.general collection Related to Ansible Collections work feature This issue/PR relates to a feature request. has_issue macos macOS community module This issue/PR relates to a module. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_contributor This PR is the first contribution by a new community member. pep8 Here be dragons stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. system System category test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.