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

Please document the hash_behaviour=merge deprecation decision and recommended replacement #73089

Closed
jerrac opened this issue Dec 31, 2020 · 15 comments · Fixed by #73328
Closed
Labels
affects_2.11 feature This issue/PR relates to a feature request. needs_info This issue requires further information. Please answer any outstanding questions. needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly. support:core This issue/PR relates to code supported by the Ansible Engineering Team.

Comments

@jerrac
Copy link

jerrac commented Dec 31, 2020

I only just found out about hash_behavior=merge being deprecated today. And then promptly discovered that any issues talking about it were locked, and I could find no explanation of why it's deprecated, beyond that it is supposedly fragile.

To be blunt, I don't agree with the decision. In fact I've always thought it was rather weird that it isn't the default behavior. I have been successfully relying on it for multiple years with dozens of servers. I've never had any major issues with it. So I cannot see the evidence that it is "fragile".

Since the combine filter is the recommended replacement, I did read those docs in detail.

Frankly, how is that a replacement? It merges two different hashes. That is entirely different than combining hashes with the same names and keys from group_vars, host_vars, role defaults, role vars, playbooks, the command line, and I'm sure other places I'm forgetting...

If there is a way, I'm not seeing it right now. And even then, you trade the complexity of making sure your group and host vars are set up right, for the complexity of using a filter the correct way. You also break the functionality for anyone who uses roles not built for it. (Side note, filter syntax is horrid. It is not easy to read. That is a serious problem when trying to debug things.)

I get that it is far too late to change things, but please respect those of us who are having their floors dropped out from under them by this decision, enough to give us an explanation.

I'd also ask that you go unlock comments on the issues you've locked. #72669 and #72421 are the ones I found. Or at least leave a comment directing us to a proper location to discuss things. The way things are right now is disrespectful. Not something I expected from this project.

Finally, a detailed post on how to convert from hash_behaviour=merge to using the combine filter should be written. The combine filter docs are not enough. If you are going to tell people they need to change how they do something, you need to provide them with the knowledge to make the change. (If you're actually telling us there is no way to do what we want, then say so. We'll go find a better tool.)

@ansibot
Copy link
Contributor

ansibot commented Dec 31, 2020

Files identified in the description:
None

If these files are incorrect, please update the component name section of the description or use the !component bot command.

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Dec 31, 2020

@jerrac: Greetings! Thanks for taking the time to open this issue. In order for the community to handle your issue effectively, we need a bit more information.

Here are the items we could not find in your description:

  • component name

Please set the description of this issue with an appropriate template from:
https://github.com/ansible/ansible/tree/devel/.github/ISSUE_TEMPLATE

click here for bot help

@ansibot ansibot added affects_2.11 feature This issue/PR relates to a feature request. needs_info This issue requires further information. Please answer any outstanding questions. needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Dec 31, 2020
@felixfontein
Copy link
Contributor

If you just need the functionality for host_vars and group_vars, and not in other places (like roles), this can be simply solved by creating a vars plugin that does the same than https://github.com/ansible/ansible/blob/devel/lib/ansible/plugins/vars/host_group_vars.py except that it always merges dictionaries. Then you can remove the original host_group_vars from the allowlist and enable this one instead - problem solved. (Alternatively, you could have both vars filters enabled, and make the new one use directories group_vars.merged and host_vars.merged instead. And if such a plugin is available in a collection, it would be easy for users to install and use it.)

For roles, it should be pretty simple to work around this with the combine filter. If you have defaults:

my_role_option:
    var1: default1
    more:
        var2: default2

you could simply rename this to MY_ROLE_OPTION_DEFAULTS, add a default my_role_option: {}, and in the beginning of the role use set_fact to combine MY_ROLE_OPTION_DEFAULTS and my_role_option with the combine filter into a new fact that you can use during the role.

This cannot be used for values directly specified on the command line (for ansible-playbook and ansible) I guess.

Are there important use-cases that are not covered by the above two work-arounds?

@felixfontein
Copy link
Contributor

I just noticed that @b-abadie already suggested a modified vars plugin in #63300 (comment). This is now possible with collections, as long as you are avoiding Ansible 2.9 (which does not support vars plugins in collections) or earlier. ansible-base 2.10 supports vars plugins in collections.

@jerrac
Copy link
Author

jerrac commented Dec 31, 2020

@felixfontein Thanks for the tips. I'll take some time to see what I can do with them.

That said, my main problem is the lack of communication on the subject, and the way most/all the issues have been locked. I'd get it if there had been a solid discussion already, and there wasn't any point in rehashing (pun, ha) it. But I sure haven't been able to find any evidence of that discussion, and if there was one, I'd expect the devs to leave a comment pointing people to it when closing/locking issues.

The closest thing I found to reasoning was the reddit post linked to in that pull request. I did find that yesterday. I didn't find that it seems to be why this change was made... What I don't get is why that post seems to have been all the discussion needed to justify making people like me either spend many hours rewriting our code, or many hours learning a new tool...

I am more than a little frustrated by this. I only just got team members at my workplace to really start using configuration management. It's taken me years. I really don't want to go tell them, "Sorry, all that work you did needs to be rewritten due to one reddit post by an Ansible contributor."

The biggest reason I chose Ansible over the other tools years ago was that when I asked questions in the Google Group, Michael DeHaan responded directly. And my other interactions at the time were generally very friendly, even when asking newbie questions. Seeing issues locked without comment is a stark difference to those early days. Ansible has largely just worked for me for years now, hence why I didn't hear about this change earlier. So the way all this has been handled is worrisome, if this large of a change can be made without discussion, and community attempts to discuss it are shut down, how can I trust Ansible? Where did that friendliness go?

Anyway, those are some of my thoughts on this. Maybe it's just me, or maybe others are feeling similar. I just want to share because if I don't, then I'm not even giving Ansible a chance to reassure me or otherwise respond.

@felixfontein
Copy link
Contributor

We discussed this topic a bit on today's public project meeting (logs: https://meetbot.fedoraproject.org/ansible-meeting/2021-01-05/ansible_core_public_irc_meeting.2021-01-05-19.07.log.html). Unfortunately nobody was around who actually uses that feature.

@jerrac
Copy link
Author

jerrac commented Jan 5, 2021

@felixfontein Thanks! Sorry for not being there, I really thought it was going to be tomorrow... :\

I will do my best to make it to the next one.

@felixfontein
Copy link
Contributor

The topic was again skipped today. The next meeting is next Tuesday at 19:00 UTC.

@jerrac
Copy link
Author

jerrac commented Jan 7, 2021

Sorry about missing again.

Is there anything we can talk about here that would help?

@sivel
Copy link
Member

sivel commented Jan 7, 2021

If you just want documentation, you can start by reading https://www.reddit.com/r/ansible/comments/a7x92q/in_which_case_hash_behaviourmerge_can_become/ec8mf7f/

If you wish the decision to be reversed, you will need to attend the IRC meeting with the intention of convincing the core team to reverse the decision.

@b-abadie
Copy link
Contributor

b-abadie commented Jan 7, 2021

I fail to see what this reddit comment is supposed to document. Is it "why was the hash_behaviour=merge deprecated?" or "what are hash_behaviour=merge's alternatives?" ?

@sivel
Copy link
Member

sivel commented Jan 7, 2021

I fail to see what this reddit comment is supposed to document

Why hash_behavior=merge is bad.

what are hash_behaviour=merge's alternatives?

As far as alternatives, there is no drop in replacement. The deprecation warning lists that it will require explicit playbook author decisions when to merge variables with different variable names via the jinja2 combine filter, which will require rewriting playbooks.

To quote from another issue that was raised:

We are explicitly removing a 1 stop method of merging data, in favor of requiring authors to make individual explicit decisions for variable merging. This is exactly what was intended.

We do not plan to resurrect such a general variable merging feature.

There is no non-refactoring option.

You may elect to stay on older versions of Ansible that supports the functionality you desire. Ansible 2.9 will be supported until 2023, and Ansible 2.12 may be supported until potentially 2026

@b-abadie
Copy link
Contributor

b-abadie commented Jan 7, 2021

Why hash_behavior=merge is bad.

Here are quotes from the reddit post that, in my opinion, relate to "why hash_behavior=merge is bad" but fail to provide objective information as to "why" :

This feature is a mistake

This one, in particular, has to be avoided at all costs.

This is evil.

Here are quotes that provide an embryo of information as to why it is so bad :

It encourages bad practices.

and can lead to catastrophic failures

If you need to use it, it means something is really wrong in your inventories.

Which bad practice ? Which catastrophic failures ? What is "really wrong" with the inventories ?

If you want a more constructive response from someone that was using this as a core feature (before I dropped ansible altogether for new projects because of this unexplained unilateral deprecation), see my unanswered comment #63300 (comment) from more than a year ago.

@jerrac
Copy link
Author

jerrac commented Jan 7, 2021

That reddit comment only says that the person who originally got hash_behavior=merge into Ansible now regrets it. One person changing their mind on something is not enough justification for causing hours of unexpected extra work. Much of it paid.

If hash_behavior=merge is actually bad, then there should be people who have seen it cause problems, and can explain how copying and pasting code hundreds of times is a better solution. Even better would be if they can associate a financial cost with it.

As for convincing the core team not to remove something, I've seen several concrete examples on how hash_behavior=merge saves time and keeps things simpler. I'd also argue that removing a feature should require that the ones advocating for the removal prove it's a good idea. Not the other way around.

If you look at my profile, you can find the roles I've written. Using those roles, plus hash_behavior=merge, means I can change 1 or 2 files, and have that change easily applied to dozens of servers. Take away hash_behavior=merge, and I end up multiplying how long it takes to apply a change by however many servers I have.

(FYI, at my workplace, most of our servers are snowflakes, they all run different apps, so I end up with lots of stuff in host_vars instead of group_vars...)

Anyway, I will keep trying to attended the meetings, but I'm still confused on why we can't hold the discussion here...

@samdoran samdoran removed the needs_triage Needs a first human triage before being processed. label Jan 7, 2021
@dwt
Copy link

dwt commented Jan 18, 2021

I would like to say that as an avid user of hash merging I would really like to see a more precise deprecation message that tells men where I am actually using merging, (still). I have already begun the process to get rid of merging as a requirement, but it's quite deeply rooted in my workflow.

It seems the easiest way to get merging back after it is deprecated is to use the inherent merging of the different inventory sources. So, I go from something like this:

# group_vars/all.yml
backup:
  global_encryption_key_id: a234cde
#host_vars/host.yml
backup:
  encryption_key_id: b234cde

To this:

# group_vars/all.yml
backup_global_encryption_key_id: a234cde
#host_vars/host.yml
backup_encryption_key_id: b234cde

I have to say that I find dictionaries as namespaces a heckin good idea (ok, I'm a python programer) and deeply regret that they are not supported anymore. I am fairly sure that ansible will not be able to stop allowing merging of different variable sources as that is way too deeply ingrained in it's DNA.

I have to say I found merging a really nice extension of this basic behaviour that allowed nice and clean grouping of variables in dicts as namespaces and too never understood why that wasn't the default as it simplifies the whole mental model a lot (as with merging, not only the first level of variables is merged, but all of them, making - for me - for a much simpler mental model.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.11 feature This issue/PR relates to a feature request. needs_info This issue requires further information. Please answer any outstanding questions. needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants