firewalld - announce breaking changes

[Akasurde]

SUMMARY

• masquerade and icmp_block_inversion will be changed from str to bool

Signed-off-by: Abhijeet Kasurde [email protected]

ISSUE TYPE

• Docs Pull Request

COMPONENT NAME

changelogs/fragments/firewalld_breaking_change.yml

[Akasurde]

@vrindle @justjais Could you please review this? Thanks in advance.

[Andersson007]

I think the sanity failures relate to ansible-collections/overview#45 (comment)

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

[Andersson007]

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.

[Akasurde]

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.

[Akasurde]

Sanity fixes via #250

[saito-hideki]

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.

@Akasurde @Andersson007 May I try to fix this? if you guys are not working on it yet :)

[Akasurde]

@saito-hideki Yes sure.

[vrindle]

@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.

[richm]

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.

[vrindle]

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.

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.

[saito-hideki]

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.

[vrindle]

@saito-hideki That would be a good idea. I am fine with that.

[Andersson007]

The bot closed it (as there was "Fixes .." statement in the warning PR). Reopened.

[Andersson007]

@Akasurde @saito-hideki as we discussed today in the Warning PR, we can merge this PR too

[Andersson007]

LGTM

[maxamillion]

@Andersson007 revisiting, does this still need to be merged?

[maxamillion]

cc @Akasurde

[Andersson007]

@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?

[maxamillion]

Closing and reopening to kick CI.

[softwarefactory-project-zuul]

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/8bc5255d8cbf49e1b5d311dc13d4577c

✔️ ansible-galaxy-importer SUCCESS in 4m 48s
✔️ build-ansible-collection SUCCESS in 7m 41s