allow-firewalld-take-multiple-input

[sunshine69]

The currentl firewalld module does not take multiple values such as
source or interface, etc..

There are many cases that we need to pass multi volume to the module
rather than flatening the input so I implement it in this PR.

This change is backward compatible, that is the behaviour wont change after this change,
an existing user uses single value will work as is.

SUMMARY

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

firewalld

ADDITIONAL INFORMATION

[sunshine69]

I just dont see why the check failed - the error logs does not have it so I dont know what need to improve. Please advise.

[aminvakil]

Thanks for creating this PR!

Also according to https://docs.ansible.com/ansible/latest/community/development_process.html#creating-a-changelog-fragment "The file name should include the PR number and a description of the change."

So it should be something like 160-firewalld_multiple_input_values.yml.

[sunshine69]

Thanks for creating this PR!

Also according to https://docs.ansible.com/ansible/latest/community/development_process.html#creating-a-changelog-fragment "The file name should include the PR number and a description of the change."

So it should be something like 160-firewalld_multiple_input_values.yml.

Thanks, I did all you said and the check still failed!

[aminvakil]

@sunshine69 That's odd, I will investigate and ask others too later today.

[tadeboro]

Thank you for your contribution, @sunshine69!

Sanity test fail on the pep8 check: https://51388069b44786266e9b-30750299912a16616e1da61098cd5e85.ssl.cf1.rackcdn.com/160/1d9f2183434e5a375ae4ce2a13329c72bd6c8228/check/ansible-test-sanity-docker/8c8f4de/job-output.html#l1300

As for the changelog fragment check, it seems that it only looks for the changelog addition in the last commit. So I would advise you to fix the pep8 issues, squash all your commits into one, and then force-push to your branch.

[tadeboro]

I went through the code and I started to wonder that maybe it would be better to convert the type: str to type: list, elements: str and then let Ansible handle the conversion.

This change would allow the user to specify:

1. a single string like now and Ansible will convert this to a list with a single element,
2. a list of strings, and
3. a comma-searated string that Ansible will convert to list.

[sunshine69]

I went through the code and I started to wonder that maybe it would be better to convert the type: str to type: list, elements: str and then let Ansible handle the conversion.

This change would allow the user to specify:

1. a single string like now and Ansible will convert this to a list with a single element,

2. a list of strings, and

3. a comma-searated string that Ansible will convert to list.

Wonder the benefit of this approach?
I saw some module handle that way as well, but I am not sure if it is correct way, as I think it might be not:

• Explicit (Explicit better than Implicit) as user have too many options to type in, can be string, a list and that can make a confusion
• A minor increase in complexity

But it might be intuitive, as first user will naturally think the input should be a list so naturally it should be a list.
And because we have to make backward compatible so we still need to allow string to be in

Don't know, but I think it is good that way, I will implement it soon.

[sunshine69]

Thank you for your contribution, @sunshine69!

Sanity test fail on the pep8 check: https://51388069b44786266e9b-30750299912a16616e1da61098cd5e85.ssl.cf1.rackcdn.com/160/1d9f2183434e5a375ae4ce2a13329c72bd6c8228/check/ansible-test-sanity-docker/8c8f4de/job-output.html#l1300

As for the changelog fragment check, it seems that it only looks for the changelog addition in the last commit. So I would advise you to fix the pep8 issues, squash all your commits into one, and then force-push to your branch.

Did the squash and other stuff, but I ran pep8 locally and did not see any errors like no space after , so finger crossed!

[sunshine69]

OK Now I got stuck then. Changelog is there but it still complains that changelog does not exists.
And for all other Pep8 it is not my change causing it.

Maybe you guys need to fix the checking logic somehow - and fixing the rest of the pep8 error.

[sunshine69]

Can someone please look into the build errors please?

[Akasurde]

@sunshine69 I looking into the error.

[Akasurde]

@sunshine69 #168 needs to be merged before this. I re-triggered CI, will ping you once that is merged.

[saito-hideki]

@sunshine69 @aminvakil @tadeboro @Akasurde
Sorry if I misunderstood. But If a new argument specification method becomes possible, wouldn't it be necessary to have an example and some integration tests?

[maxamillion]

Hello! Apologies for the lag time on this, if this could get some examples and test cases as mentioned in the last comment then I would gladly merge. Thank you!