ansible.posix
ansible.posix copied to clipboard
ansible-collections
firewalld - announce breaking changes
SUMMARY
masqueradeandicmp_block_inversionwill be changed fromstrtobool
Signed-off-by: Abhijeet Kasurde [email protected]
ISSUE TYPE
- Docs Pull Request
COMPONENT NAME
changelogs/fragments/firewalld_breaking_change.yml
I think the sanity failures relate to https://github.com/ansible-collections/overview/issues/45#issuecomment-897333102
ERROR: Found 4 pylint issue(s) which need to be resolved:
ERROR: plugins/modules/synchronize.py:365:25: disallowed-name: Disallowed name "_"
ERROR: plugins/modules/synchronize.py:369:29: disallowed-name: Disallowed name "_"
ERROR: tests/sanity/ignore-2.12.txt:1:1: ansible-test: Ignoring 'blacklisted-name' on 'plugins/modules/synchronize.py' is unnecessary
ERROR: tests/unit/mock/loader.py:49:4: arguments-renamed: Parameter 'file_name' has been renamed to 'path' in overridden 'DictDataLoader._get_file_contents' method
Can we put a warning in the code when, say, the string doesn't look like a boolean, print the warning? The collection is supported and many users don't read changelogs.
OK.
Sanity fixes via https://github.com/ansible-collections/ansible.posix/pull/250
@Akasurde @saito-hideki I think we are going to be replacing the Ansible Posix Firewalld module with the current module in the Firewalld system role in the new release. This is because the system role already has icmp-block-inversion and masquerade as booleans instead of strings. It also has the ability to do multiple transactions. So making this change in the code base isn't paramount because eventually those who want the breaking changes will be switching over to the new release which uses the Firewalld system role module and people who want to keep the functionality as is can use the current version of Ansible Posix Firewalld. If we want it so that minimal damage happens, we keep the code base as is and just put all the changes into the new release (which will use the Firewalld system role). However if you are fine with breaking changes, you can make the change in the existing code base as it will ease the transition into the new release easier.
I guess it depends on how many users are using the masquerade and icmp_block_inversion parameters with a value other than "yes" or "true" (or any boolean YAML value). If that number is very, very small, then this change is pretty safe. And this change will help make the transition to the new firewalld module easier.
I guess it depends on how many users are using the
masqueradeandicmp_block_inversionparameters with a value other than "yes" or "true" (or any boolean YAML value). If that number is very, very small, then this change is pretty safe. And this change will help make the transition to the new firewalld module easier.
Yes this is accurate, however there isn't any way to gauge how many customers are using other values so maybe it might be a better idea to keep it as is. We could ask customers or check source code on github to double check if workflows don't break as a result of these changes.
Hi @Akasurde @vrindle @richm At this time, what about keeping the "str" type and just showing a warning message that it will change to a Boolean value in a future release of firewalld module if masquerade or icmp_block_inversion is specified like @Andersson007 pointed out.
The bot closed it (as there was "Fixes .." statement in the warning PR). Reopened.
@Akasurde @saito-hideki as we discussed today in the Warning PR, we can merge this PR too
@maxamillion i don't know, I hasn't been a maintainer of this repo:) Thanks for bringing attention to this PR! @saito-hideki would you like to merge this?
Build succeeded. https://ansible.softwarefactory-project.io/zuul/buildset/8bc5255d8cbf49e1b5d311dc13d4577c
:heavy_check_mark: ansible-galaxy-importer SUCCESS in 4m 48s :heavy_check_mark: build-ansible-collection SUCCESS in 7m 41s
![softwarefactory-project-zuul[bot] avatar](https://avatars.githubusercontent.com/in/6924?v=4 "Published by a pub.dev verified publisher"){kind=small}