-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add new lib and filter for merging datas in form of list or dict. #4799
Conversation
plugins/filter/merge_data.py
Outdated
name: merge_data | ||
short_description: Merge dict or lists by ensuring the comparing datas are | ||
presents, absents or identics. | ||
version_added: 5.0.2 |
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.
version_added: 5.0.2 | |
version_added: 5.2.0 |
(5.0.2 would be a bugfix release anyway, that cannot have new features.)
plugins/filter/merge_data.py
Outdated
positional: expected, state, list_diff_type | ||
options: | ||
expected: | ||
type: dict|list |
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 is an invalid type. Use raw
if you cannot decide on one.
type: dict|list | |
type: raw |
plugins/module_utils/data_merge.py
Outdated
if not isinstance(current, (dict, list)) or not isinstance(expected, (dict, list)): | ||
raise ValueError('Only dict or list can be merged, {0} and {1} given.'.format(type(current), type(expected))) | ||
if self.merge_type == 'identic': | ||
return deepcopy(expected) |
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 don't think calling deepcopy
is a good idea, since for a filter you can pass in arbitrary ansible-core objects that probably shoudn't simply be copied.
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.
The filter use DataMerge
that I designed to be reusable in various context, not only in this filter.
I'm afraid that if I remove the deepcopy
in get_new_merged_data
, get_new_merged_dict
and get_new_merged_list
we got unexpected behaviours later.
Firstly, we get a problem when we use merge_type='absent
. get_new_merged_dict
call itself recursively for each dict
element that it contains. If we do not return deepcopy
for the result we modifing the original object by removing some element on it, and we get problems when we try to continus to iterating over its elements (concretely, it raises RuntimeError: dictionary changed size during iteration
).
Secondly, when we call get_new_merged_data
, get_new_merged_dict
and get_new_merged_list
, we expect to get a new object. If we remove deepcopy
it possible to get one of current
or expected
object, depending on the used params, modified by the call of the method. If I need to reuse later one of current
or expected
I can't because the original value has been replaced by the result. It's bit tricky...
I don't see very clearly what ansible-core objects can be passed to the filter. Please, can you give me some sample to help me that understand this. Maybe I can treat the problem in merge_data
, the function called by the filter, avoiding modifying anything in DataMerge
.
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'm afraid that if I remove the
deepcopy
inget_new_merged_data
,get_new_merged_dict
andget_new_merged_list
we got unexpected behaviours later.
I do not mean simply removing deepcopy
without replacement, but writing code which handles this correctly.
I don't see very clearly what ansible-core objects can be passed to the filter. Please, can you give me some sample to help me that understand this. Maybe I can treat the problem in
merge_data
, the function called by the filter, avoiding modifying anything inDataMerge
.
For example hostvars
is of type ansible.vars.hostvars.HostVars
, and hostvars[inventory_hostname]
is of type ansible.vars.hostvars.HostVarsVars
. Also lookups and filters can return arbitrary Python objects.
You should probably look at how merge_hash
is implemented in lib/ansible/utils/vars.py in https://github.com/ansible/ansible/
'data_expected': ['A', 'C'], | ||
}, | ||
] | ||
DATA_MERGE_TEST_CASE_LIST_IDS = (item['id'] |
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.
Create a list instead of a generator.
DATA_MERGE_TEST_CASE_LIST_IDS = (item['id'] | |
DATA_MERGE_TEST_CASE_LIST_IDS = [item['id'] |
(Ending needs to be changed as well.)
plugins/filter/merge_data.py
Outdated
version_added: 5.0.2 | ||
author: DEMAREST Maxime (@indelog) | ||
description: | ||
- Merge two datas, current and expected in form of dict or list. |
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.
- Merge two datas, current and expected in form of dict or list. | |
- Merge two data, current and expected in form of dict or list. |
data
is already plural, but I don't think you can say "two data" (https://www.merriam-webster.com/dictionary/data#usage-1). How about
- Merge two datas, current and expected in form of dict or list. | |
- Merge two data structures, current and expected in form of dict or list. |
?
plugins/filter/merge_data.py
Outdated
description: | ||
- Merge two datas, current and expected in form of dict or list. | ||
- You can have mixed data like dict->list or list->dict. If the two type | ||
are different, the expected datas are always returned. |
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 don't think I understand this. Do you mean that one argument is a dictionary, while the other a list? And with "expected", do you mean the input named "expected" (should we written as I(expected)
), or the english word "expected"?
plugins/filter/merge_data.py
Outdated
- For the list, you have two type of diff. You can check if the expected | ||
datas are present in a list on the current datas or you can | ||
check a value at the specific index of the list. | ||
positional: expected, state, list_diff_type |
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.
positional: expected, state, list_diff_type | |
positional: expected |
I would keep state
and list_diff_type
as keyword arguments.
plugins/module_utils/data_merge.py
Outdated
Getting the merge of two element if it can't be sure that they have the | ||
same type. | ||
""" | ||
if not isinstance(current, (dict, list)) or not isinstance(expected, (dict, list)): |
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.
Instead of list
and dict
you should use Sequence
and Mapping
(from ansible.module_utils.common._collections_compat import Mapping, Sequence
).
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.
If we do not ensure that the type of element is exactly a list
or a dict
, we can get some unexpected behaviour. For example, for get_new_merged_list
, by replacing isinstance(current, list) or not isinstance(expected, list)
by isinstance(current, Sequence) or not isinstance(expected, Sequence)
we can get a str
as current
and expected
which will result to an error.
I think I can integrate Mapping
or Sequence
as parameter rather than list
or dict
but DataMerge
will be more complicated and heavy because we will have more case to treat. Is really useful to do that ?
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.
If we do not ensure that the type of element is exactly a
list
or adict
, we can get some unexpected behaviour. For example, forget_new_merged_list
, by replacingisinstance(current, list) or not isinstance(expected, list)
byisinstance(current, Sequence) or not isinstance(expected, Sequence)
we can get astr
ascurrent
andexpected
which will result to an error.
Well, then make sure it is not isinstance
of string_types
(from ansible.module_utils.six import string_types
).
I think I can integrate
Mapping
orSequence
as parameter rather thanlist
ordict
butDataMerge
will be more complicated and heavy because we will have more case to treat. Is really useful to do that ?
Since this is used by a plugin, you can easily get data structures in here that are lists or dictionaries, but not of type list
or dict
. So yes, it is very useful to handle these correctly.
I've done all thinks indicated on comments except two on which I encounter problems. I provided details on comments responses. My English is weak. I've difficulty to express clearly complicated things, sorry for that. I fixed indicated grammar problems and tried to rewrite some parts of the documentation. I expect that is more understandable like this. I've also splited some elements of filter documentation in docs fragments which can be reusable by modules that may use |
The test
The test
The test
|
plugins/filter/merge_data.py
Outdated
version_added: 5.2.0 | ||
author: DEMAREST Maxime (@indelog) | ||
description: | ||
- Compare data structure provided in I(_input) with data structure provided |
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.
Hmm, I don't think "compare" is the correct verb here. The filter returns a modified copy of the original data. To compute the result it does compare something, but more importantly is that it returns a modified copy.
plugins/filter/merge_data.py
Outdated
|
||
DOCUMENTATION = r''' | ||
name: merge_data | ||
short_description: Compare one data structure provided in I(_input) with an |
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.
The short description should be something really short. It is what is printed next to the plugin name in https://docs.ansible.com/ansible/devel/collections/community/general/. (It also should not end with a period.)
plugins/filter/merge_data.py
Outdated
EXAMPLES = r''' | ||
- name: Ensure that the expected data will be present with lists merged by values. | ||
ansible.builtin.debug: | ||
msg: "{{ current|community.general.merge_data(expected, state='present') }}" |
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.
The state
parameter feels really strange for a filter. I would rather expect an operation here, as a filter doesn't ensure idempotency, but modifies something. Especially the value identic
seems wrong, doesn't it simply mean "return expected
"?
plugins/module_utils/data_merge.py
Outdated
Getting the merge of two element if it can't be sure that they have the | ||
same type. | ||
""" | ||
if not isinstance(current, (dict, list)) or not isinstance(expected, (dict, list)): |
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.
If we do not ensure that the type of element is exactly a
list
or adict
, we can get some unexpected behaviour. For example, forget_new_merged_list
, by replacingisinstance(current, list) or not isinstance(expected, list)
byisinstance(current, Sequence) or not isinstance(expected, Sequence)
we can get astr
ascurrent
andexpected
which will result to an error.
Well, then make sure it is not isinstance
of string_types
(from ansible.module_utils.six import string_types
).
I think I can integrate
Mapping
orSequence
as parameter rather thanlist
ordict
butDataMerge
will be more complicated and heavy because we will have more case to treat. Is really useful to do that ?
Since this is used by a plugin, you can easily get data structures in here that are lists or dictionaries, but not of type list
or dict
. So yes, it is very useful to handle these correctly.
plugins/module_utils/data_merge.py
Outdated
if not isinstance(current, (dict, list)) or not isinstance(expected, (dict, list)): | ||
raise ValueError('Only dict or list can be merged, {0} and {1} given.'.format(type(current), type(expected))) | ||
if self.merge_type == 'identic': | ||
return deepcopy(expected) |
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'm afraid that if I remove the
deepcopy
inget_new_merged_data
,get_new_merged_dict
andget_new_merged_list
we got unexpected behaviours later.
I do not mean simply removing deepcopy
without replacement, but writing code which handles this correctly.
I don't see very clearly what ansible-core objects can be passed to the filter. Please, can you give me some sample to help me that understand this. Maybe I can treat the problem in
merge_data
, the function called by the filter, avoiding modifying anything inDataMerge
.
For example hostvars
is of type ansible.vars.hostvars.HostVars
, and hostvars[inventory_hostname]
is of type ansible.vars.hostvars.HostVarsVars
. Also lookups and filters can return arbitrary Python objects.
You should probably look at how merge_hash
is implemented in lib/ansible/utils/vars.py in https://github.com/ansible/ansible/
I've been a bit long... I've done some changes but, the background remain the same. Firstly, i have solved the I considered a very simple solution (that i hope not be too naive). To get a new To to this i needed to refactor the The As a 5.2.0 release was published, i change the The doc fragment |
@felixfontein |
@indelog This PR was evaluated as a potentially problematic PR for the following reasons:
Such PR can only be merged by human. Contact a Core team member to review this PR on IRC: |
I fixed all that is pointed in your comments. I have renamed the filter into I'll let you tell me if other things are wrong. |
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.
Please see further comments.
@@ -21,6 +21,7 @@ jobs: | |||
provide-link-targets: | | |||
ansible_collections.ansible.builtin.dict2items_filter | |||
ansible_collections.ansible.builtin.path_join_filter | |||
ansible_collections.ansible.utils.update_fact_module |
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.
Why is this PR touching GH workflows?
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.
To avoid documentation build errors. Only references in this list are allowed to be used in module docs. As long as
seealso:
- module: ansible.utils.update_fact
description: Do something similar in another way.
is in the plugins/filter/structure_merging.py docs, this modification is needed.
`present` parameter is a boolean that set if items in `changes` be used | ||
to adding/updating or to removing items from `base`. | ||
- When `True`, it acts to add or update items. | ||
- When `False`, it acts to remove items. |
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.
The name and semantics of this parameter/feature seems off: when we say it is not present, we actually mean that it will be removed. I think this would make more sense as action
having the choices ADD_OR_UPDATE
and REMOVE
.
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.
And you may hate me for picking on the name again, but I think merging is defeated by present=False
- we are actually subtracting rather than merging.
In fact, my general feeling is that this class is more complicated than it needs to be and, to a great length, that derives from the fact that it is trying to do many different things in one single class. I think this should be broken down into something that handles dict
and something that handles list
, and maybe we should break it further down into something that merges stuff and something that removes stuff (maybe this last one is a bit overkill).
* get this result : | ||
`{'A': {'AB': '2'}, 'B': ['4'], 'C': '5'}` | ||
|
||
`merge_seq_by_index` is a boolean that set if `Sequence` items are |
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.
maybe name it keep_seq_index
or keep_list_index
? Or maybe match_seq_index
- probably better.
* or if using `present=False`, get : | ||
`['A', {'C': '1'}]` | ||
|
||
`keep_empty` parameter defines if emptied `Sequence` or `Mapping` items |
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.
maybe keep_empty_collections
or keep_empty_containers
?
when is needed to ensure that a key will be removed. | ||
|
||
Example that ignoring items at a specific position with | ||
`remove_none=False` and `merge_seq_by_index=True` : |
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.
There is so much documentation in this file that maintaining it has become an issue: the cost of change is too high because you have to go back to all the docs and perform the change in every single occurrence. Instead of over-documenting, it is better to give more meaningful names to the variables/parameters/functions so that their purpose become self-evident.
remove_none
is probably a previous incarnation of keep_null
?
# Ignoring value of changes[k] when it None permit to avoid to | ||
# update this item when it explicitly skipped with | ||
# `merge_seq_by_index=True`. | ||
if k not in keys_changes or changes[k] is None: |
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.
Simpler as:
if k not in keys_changes or changes[k] is None: | |
if changes.get(k) is None: |
base_dict = self._convert_generic_sequence_to_dict(base) | ||
changes_dict = self._convert_generic_sequence_to_dict(changes) | ||
res_dict = self._get_new_dict_with_changes_present(base_dict, changes_dict) | ||
new = self._convert_generic_mapping_to_list(res_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.
Converting back and forth has a couple of problems there:
- increased memory usage (to hold the same data), which might become a problem if someone decides to use this for larger datasets
- lists, by definition, maintain order, and the conversion method back to list does not do that
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.
It could be solved by something (untested) like:
base_dict = self._convert_generic_sequence_to_dict(base) | |
changes_dict = self._convert_generic_sequence_to_dict(changes) | |
res_dict = self._get_new_dict_with_changes_present(base_dict, changes_dict) | |
new = self._convert_generic_mapping_to_list(res_dict) | |
for base_elem, chg_elem in zip(base, changes): | |
res_elem = self._choose_merge_method(base_elem, chg_elem, present=True)(base_elem, chg_elem) | |
res.append(res_elem) |
`{'A': '1', 'C': {'CA': '3'}}` | ||
""" | ||
|
||
if self._both_are_assimilable_to_dict(base, changes): |
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.
If I may suggest, instead of repeating this if block multiple times in the code, make a small static method that performs that logic and returns the right method to you, then here that method would be assigned to _get_method
(or a better name as suggested before), then lines 181-187 would actually run whatever comes out, same for 194-204, 210-215, and 221-226.
For the sake of argument, will call that function _choose_merge_method(base, change, present)
.
return new | ||
|
||
@staticmethod | ||
def _both_are_assimilable_to_dict(base, changes): |
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.
too verbose a name... maybe _both_dict
, so that we can code it like:
if _both_dict(base, changes):
# blablabla
if (isinstance(base, string_types) or isinstance(changes, string_types)): | ||
return False | ||
return (isinstance(base, Sequence) and isinstance(changes, Sequence)) |
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 suggest you use ansible.module_utils.common.collections.is_sequence()
for simplification.
# Copyright (c) 2022 , DEMAREST Maxime <maxime@indelog.fr> | ||
# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) |
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.
# Copyright (c) 2022 , DEMAREST Maxime <maxime@indelog.fr> | |
# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) | |
# Copyright (c) 2022, DEMAREST Maxime <maxime@indelog.fr> | |
# GNU General Public License v3.0+ (see LICENSES/GPL-3.0-or-later.txt or https://www.gnu.org/licenses/gpl-3.0.txt) | |
# SPDX-License-Identifier: GPL-3.0-or-later |
# Copyright (c) 2022 , DEMAREST Maxime <maxime@indelog.fr> | ||
# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) |
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.
# Copyright (c) 2022 , DEMAREST Maxime <maxime@indelog.fr> | |
# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) | |
# Copyright (c) 2022, DEMAREST Maxime <maxime@indelog.fr> | |
# GNU General Public License v3.0+ (see LICENSES/GPL-3.0-or-later.txt or https://www.gnu.org/licenses/gpl-3.0.txt) | |
# SPDX-License-Identifier: GPL-3.0-or-later |
# Copyright: (c) 2022, DEMAREST Maxime <maxime@indelog.fr> | ||
# GNU General Public License v3.0+ | ||
# (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) |
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.
# Copyright: (c) 2022, DEMAREST Maxime <maxime@indelog.fr> | |
# GNU General Public License v3.0+ | |
# (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) | |
# Copyright (c) 2022, DEMAREST Maxime <maxime@indelog.fr> | |
# GNU General Public License v3.0+ (see LICENSES/GPL-3.0-or-later.txt or https://www.gnu.org/licenses/gpl-3.0.txt) | |
# SPDX-License-Identifier: GPL-3.0-or-later |
@@ -0,0 +1,2 @@ | |||
shippable/posix/group3 |
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.
shippable/posix/group3 | |
# Copyright (c) 2022, DEMAREST Maxime <maxime@indelog.fr> | |
# GNU General Public License v3.0+ (see LICENSES/GPL-3.0-or-later.txt or https://www.gnu.org/licenses/gpl-3.0.txt) | |
# SPDX-License-Identifier: GPL-3.0-or-later | |
shippable/posix/group3 |
# (c) 2022 DEMAREST Maxime <maxime@indelog.fr> | ||
# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) |
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.
# (c) 2022 DEMAREST Maxime <maxime@indelog.fr> | |
# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) | |
# Copyright (c) 2022, DEMAREST Maxime <maxime@indelog.fr> | |
# GNU General Public License v3.0+ (see LICENSES/GPL-3.0-or-later.txt or https://www.gnu.org/licenses/gpl-3.0.txt) | |
# SPDX-License-Identifier: GPL-3.0-or-later |
# (c) 2022 DEMAREST Maxime <maxime@indelog.fr> | ||
# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) |
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.
# (c) 2022 DEMAREST Maxime <maxime@indelog.fr> | |
# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) | |
# Copyright (c) 2022, DEMAREST Maxime <maxime@indelog.fr> | |
# GNU General Public License v3.0+ (see LICENSES/GPL-3.0-or-later.txt or https://www.gnu.org/licenses/gpl-3.0.txt) | |
# SPDX-License-Identifier: GPL-3.0-or-later |
@@ -0,0 +1,99 @@ | |||
--- | |||
|
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.
# Copyright (c) 2022, DEMAREST Maxime <maxime@indelog.fr> | |
# GNU General Public License v3.0+ (see LICENSES/GPL-3.0-or-later.txt or https://www.gnu.org/licenses/gpl-3.0.txt) | |
# SPDX-License-Identifier: GPL-3.0-or-later | |
# (c) 2022, Maxime DEMAREST <maxime@indelog.fr> | ||
# Copyright: (c) 2022, Ansible Project | ||
# GNU General Public License v3.0+ | ||
# (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) |
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.
# (c) 2022, Maxime DEMAREST <maxime@indelog.fr> | |
# Copyright: (c) 2022, Ansible Project | |
# GNU General Public License v3.0+ | |
# (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) | |
# Copyright (c) 2022, DEMAREST Maxime <maxime@indelog.fr> | |
# Copyright (c) 2022, Ansible Project | |
# GNU General Public License v3.0+ (see LICENSES/GPL-3.0-or-later.txt or https://www.gnu.org/licenses/gpl-3.0.txt) | |
# SPDX-License-Identifier: GPL-3.0-or-later |
@@ -0,0 +1,2 @@ | |||
shippable/posix/group3 |
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.
shippable/posix/group3 | |
azp/posix/3 |
(see #5344)
Hi @indelog are you still working on this? needs_info |
@russoz |
Hi @indelog please be aware that there is another PR with a similar proposition being worked on: #5533 I recommend you and this other PR author to touch base and see whether your efforts can be combined. Both PRs are about merging data structures, and it would be nice if we were not to write duplicated pieces of code to do the same stuff. |
needs_info |
@indelog This pullrequest is waiting for your response. Please respond or the pullrequest will be closed. |
@indelog You have not responded to information requests in this pullrequest so we will assume it no longer affects you. If you are still interested in this, please create a new pullrequest with the requested information. |
This follow my first PR #4686 with attempt to add new module for manage content in JSON, YAML and TOML file.
The PR was closed because it was to heavy a complicated to review and test and need to be split in several parts.
This is the firs part of new functionality that i attempt to add with
data_merge
a lib that can be used to merge tow dataset in form of dict or list by ensuring that expected datas will be present, absent or indentic to the result in function to the wanted state.It also add a new filter
merge_data
, that permit to use the functionality directly in playbook (see examples in filter documentation).NOTE
With this you can already do things similar to my final goal like this :
This is a bit rough, but this is working. I'm going anyway to try to do a complete module for doing that in better way.
NOTE 2
I've found by accident a module with a similar goal to mine (in fact, all ideas that you can have, another people get it before you) :
https://docs.ansible.com/ansible/latest/collections/ansible/utils/update_fact_module.html
I think both has different approach and be complementary by permitting to provide the most appropriate answer according with the encountered problem.