-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Conversation
6398947
to
b9560d6
Compare
After using this for a bit, defaults itself is a bit deceiving about the output of read compared to the actual types using a plist editor. For example, ~floats might show in the output as "0.5", but in an editor, it's listed as a |
Thanks @claco for this PR. This module is maintained by the Ansible core team, so it can take a while for patches to be reviewed. Thanks for your patience. |
b9560d6
to
4897eb1
Compare
Thanks @claco for this PR. Unfortunately, it is not mergeable in its current state due to merge conflicts. Please rebase your PR. When you are done, please comment with text 'ready_for_review' and we will put this PR back into review. |
4897eb1
to
f142134
Compare
ready_for_review |
Thanks @claco for this PR. This module is maintained by the Ansible core team, so it can take a while for patches to be reviewed. Thanks for your patience. [This message brought to you by your friendly Ansibull-bot.] |
Thanks @claco for this PR. This PR requires revisions, either because it fails to build or by reviewer request. Please make the suggested revisions. When you are done, please comment with text 'ready_for_review' and we will put this PR back into review. [This message brought to you by your friendly Ansibull-bot.] |
@claco A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes. When you do address the issues, please respond with ready_for_review in your comment, so that we can notify the maintainer. [This message brought to you by your friendly Ansibull-bot.] |
f142134
to
2db9d80
Compare
ready_for_review |
return [value] | ||
elif isinstance(value, list): | ||
return value | ||
elif isinstance(value, str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could use some eyes and knowledge on this. In 2.0, value came in from a dictionary in role defaults value as a dict type. Now in 2.1 it's coming through as a str. Eval works here, but I don't like it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be related to type
in the argument_spec
for an argument. If no type
is specified, str
is now assumed. Make sure your use type="dict"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @sivel
sizeup_defaults:
macosx:
- { key: CenterRelativeResizeEnabled, value: 1, type: bool }
- { key: CenterResizeType, value: 1, type: int }
- { key: CenterRelativeResizeHeight, value: 85, type: int }
- { key: CenterRelativeResizeWidth, value: 85, type: int }
- { key: ShowOverlay, value: 0, type: bool }
- { key: ShowTooltips, value: 0, type: bool }
- { key: Lower Left, type: dict, value: {key: ComboFlags, type: int, value: 10092544} }
- { key: Lower Left, type: dict, value: {key: ComboCode, type: int, value: 125} }
- { key: Lower Right, type: dict, value: {key: ComboFlags, type: int, value: 10092544} }
- { key: Lower Right, type: dict, value: {key: ComboCode, type: int, value: 124} }
- { key: Upper Left, type: dict, value: {key: ComboFlags, type: int, value: 10092544} }
- { key: Upper Left, type: dict, value: {key: ComboCode, type: int, value: 123} }
- { key: Upper Right, type: dict, value: {key: ComboFlags, type: int, value: 10092544} }
- { key: Upper Right, type: dict, value: {key: ComboCode, type: int, value: 126} }
In this case, it's the value: {} coming through as a string, even through type: dict exists. Why I don't yet get is why this happens in 2.1, but not 2.0. The playbook and default haven't changed, and the patch is roughly the same. I never needed line 180 in 2.0.
The playbook bits:
- name: Configure SizeUp Defaults
osx_defaults:
domain: com.irradiatedsoftware.SizeUp
key: '{{ item.key }}'
type: '{{ item.type }}'
value: '{{ item.value }}'
state: present
dict_add: True
with_items: '{{ sizeup_defaults.macosx }}'
when: ansible_distribution == 'MacOSX'
tags: sizeup-config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@claco In main()
where argument_spec
is defined, value
is defined as:
value=dict(
default=None,
required=False,
)
Without specifying a type
for value
, in 2.1 this gets cast to a string. If that value
can accept more than just a str
, then it needs to be updated to look like:
value=dict(
default=None,
required=False,
type='raw'
)
The change in 2.1, is that any argument in argument_spec
without a explicitly defined type, now is defaulted to str
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! Thanks! I'll give that an update.
2db9d80
to
8dfdd91
Compare
ready_for_review |
Thanks @claco for this PR. This module is maintained by the Ansible core team, so it can take a while for patches to be reviewed. Thanks for your patience. Core team: please review according to guidelines (http://docs.ansible.com/ansible/developing_modules.html#module-checklist) and comment with 'needs_revision' or merge as appropriate. [This message brought to you by your friendly Ansibull-bot.] |
Add support to osx_defaults for the `dict` type with support for additions using `dict-add`. Issue: ansible#1705
8dfdd91
to
283ea34
Compare
ready_for_review Updated PR with feedback from @sivel |
Please consider using #2392 instead as it also supports nested dictionaries and bypasses the |
Thanks @claco for this PR. Unfortunately, it is not mergeable in its current state due to merge conflicts. Please rebase your PR. When you are done, please comment with text 'ready_for_review' and we will put this PR back into review. For help on how to do this cleanly please see http://docs.ansible.com/ansible/community.html#contributing-code-features-or-bugfixes [This message brought to you by your friendly Ansibull-bot.] |
@claco A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes. When you do address the issues, please respond with ready_for_review in your comment, so that we can notify the maintainer. [This message brought to you by your friendly Ansibull-bot.] |
This repository has been locked. All new issues and pull requests should be filed in https://github.com/ansible/ansible Please read through the repomerge page in the dev guide. The guide contains links to tools which automatically move your issue or pull request to the ansible/ansible repo. |
1 similar comment
This repository has been locked. All new issues and pull requests should be filed in https://github.com/ansible/ansible Please read through the repomerge page in the dev guide. The guide contains links to tools which automatically move your issue or pull request to the ansible/ansible repo. |
Issue Type:
Plugin Name:
osx_defaults
Ansible Version:
Ansible Configuration:
Environment:
OSX El Cap 10.11
Summary:
The osx_defaults module doesn't yet support dict and dict-add methods for changing/adding defaults that are contained within values that are dicts.
Steps To Reproduce:
As an example, I'm configuring defaults for SizeUp on a new install. Many of the settings are container in dicts for each group of key commands. This command, for example, configures the combo code for the 'move window to lower left' command:
Because there is no dict/dict-add support, I have to fall back to using bash/shell commands rather than using the module.
Expected Results:
As an example, I would have expected to see something like this:
Actual Results:
What actually happened?
Issue: #1705