cp-ansible
cp-ansible copied to clipboard
confluentinc
Epic: Remove requirement for Hash Merging enabled
Ansible has decided to deprecate the hash_behavior in Ansible 2.10, and the plan is to remove it in Ansible 2.13:
- https://github.com/ansible/ansible/pull/63300
- https://www.reddit.com/r/ansible/comments/a7x92q/in_which_case_hash_behaviourmerge_can_become/ec8mf7f/
We are currently struggeling with variable precedence in our inventory, and it seems like it might be related to this setting - enforced by the requirement in this tasks: https://github.com/confluentinc/cp-ansible/blob/9c7954d5d32bb5d6aa5e84f9d9570a5ee6ba54bb/roles/confluent.common/tasks/main.yml#L2
When running our playbook, we are getting this warning:
[DEPRECATION WARNING]: DEFAULT_HASH_BEHAVIOUR option, This feature is fragile
and not portable, leading to continual confusion and misuse , use the
``combine`` filter explicitly instead. This feature will be removed in version
2.13. Deprecation warnings can be disabled by setting
deprecation_warnings=False in ansible.cfg.
I suggest that the requirement for hash_behaviour=merge is removed by following the migration path suggested by Ansible.
@erikgb Thanks for reaching out. We actually already have this on our internal roadmap to change in a future release. We've also had this raised by the community in the past, quite recently in fact, see the following closed issue:
https://github.com/confluentinc/cp-ansible/issues/458
Can you confirm the exact issue you are seeing besides the warning? Right now, the playbooks should fully work as long as hash merging is enabled, which it is in the ansible.cfg included in the repo.
@JumaX Your playbooks are probably working just fine. 😄 And I know that the playbooks currently is the only supported entrypoint. But you might have already noticed, from other issues and pull requests, that we are using the cp-ansible roles, and not the all.yml playbook - for desired state. We have created our own playbook with some additional stuff. And I cannot get the variable precedence to work the way I want, with this setting required by the cp-ansible roles. But I managed to work around it, of course.
@erikgb Understood. Can you confirm the specific ask here then? To confirm, we plan on moving away from hash merging in the future, so provided there isn't a specific ask/issue we can help with, can we close this out?
@JumaX I don't like when an open-source project has an internal Jira as master. It is either GitHub which is master, and then spill over all your findings, thoughts and musings back into GitHub issues - so that the community can follow along. Or close-source your project....
That said, you (as developers) are doing an amazing job here! Kudos! ❤️ But please take this feedback to the decicion makers in Confluent!
Closing issue as "done" - as requested.
This is still an issue, and should be fixed. Reopening to track progress in the public/community.
@erikgb The Ansible committers have decided not to remove hash merging, so this PR is no longer required. You can find details here:
https://github.com/ansible/ansible/pull/73328
https://github.com/ansible/ansible/issues/73089
https://meetbot.fedoraproject.org/ansible-meeting/2021-01-21/ansible_core_public_irc_meeting.2021-01-21-15.01.log.html
The Ansible committers have decided not to remove hash merging, so this PR is no longer required.
@JumaX Thanks for the detailed references. I wasn't aware of the un-deprecation of that variable. But it is quite debatable, and the documentation still strongly discourages use of this functionality. And for large plays - including playbooks/roles from cp-ansible, like we have, it is really sad to have to set this variable merge behavior globally. So I'll vote for fixing it, at least in the long term.
@erikgb What's interesting about this, if you read through the irc meeting notes, is that their isn't a clear reason for the feature's removal. It's one committers opinion without a lot of evidence. Even the other committers don't see an issue with it. It appears that it was generating a lot of questions from new users, but nothing more. That being said, there also doesn't appear to be a clear work around for hash merging, as combine will not solve all use cases.
I've been thinking about this, and my other concern is that we will end up with a lot of temp variables to try and work around this functionality. I could be wrong here, but I am trying to see another way we could work around hash merging but nothing is coming up thus far.
Today we had another incident where this extremely confusing setting turns out to be the root cause. Is this something that the team can prioritize to fix? @domenicbove @nsharma-git
@JumaX i would argue that this ticket itself has several posts that are a good example why this setting is problematic.
Mainly it comes down to subverting the normal expectation on how a value is arrived at, the default is 'new overrides old', while setting it to merge is 'new appends to old .. mostly ... depends on method used to set each instance of a variable'.
It also can make content incompatible, roleA was written with 'merge' while roleB was written with 'overwrite', using both in same run becomes impossible as 'variable merging' is not restricted to the role itself, but has to happen globally at all points in which you define and use a variable.
Personally, my main reason to oppose deprecation is that there is no good migration path for existing plays/roles that currently rely on it, otherwise I would be first in line to remove it.
