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.

Sign up for GitHub

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

Jump to bottom

Changed the type of the forward and masquerade options from str to bool #584

Merged
merged 1 commit into from
Oct 31, 2024
Merged

Changed the type of the forward and masquerade options from str to bool #584

merged 1 commit into from
Oct 31, 2024

Conversation

saito-hideki
Copy link
Collaborator

@saito-hideki saito-hideki commented Oct 23, 2024 •
edited
Loading

SUMMARY

The forward and masquerade options for the firewall module takes either True or False as a value.
Currently, it is defined as a string, but it should be a boolean.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME
  • ansible.posix.firewalld
ADDITIONAL INFORMATION
None

@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/5c996133a7484f178c79a6a08a05f433

✔️ ansible-galaxy-importer SUCCESS in 4m 37s
✔️ build-ansible-collection SUCCESS in 5m 27s

@saito-hideki saito-hideki marked this pull request as ready for review October 23, 2024 10:11
@saito-hideki saito-hideki self-assigned this Oct 23, 2024
@maxamillion
Copy link
Collaborator

LGTM 👍

@Andersson007
Copy link
Contributor

reviewing, please don't merge

@Andersson007
Copy link
Contributor

@saito-hideki hi, thanks for pinging!

  1. the warning

    ansible.posix/plugins/modules/firewalld.py

    Line 1137 in ad2ac28

    module.warn('The value of the forward option is "%s". '

    was added by Gerlof Fokkema 2024-06-11 that doesn't feel long enough
  2. I think this change is potentially breaking. How about changing the fragment for the change to be under breaking_changes: and releasing it in the next major release, i.e. 2.0.0 (if it's not expected soon)?

You have stable-1 for version 1.* releases, right? So if you agree with my proposal you could remove that warning as well and merge the PR to main but not backporting it to stable-1.
Thoughts folks?

@Andersson007
Copy link
Contributor

btw also spotted that the masquerade option has the same properties, there's also a warning

@saito-hideki
Copy link
Collaborator Author

@maxamillion @Andersson007 Thanks for your review comment!

@Andersson007 I agree 100% with your advice.

  1. This PR should target the main branch for the next 2.0.0 release as breaking_changes.
  2. I will remove the warning message
  3. Also, I will treat the masquerade option too.

After I address the above points, I will send the re-review request to you guys :)

@Andersson007
Copy link
Contributor

@saito-hideki sounds like a plan, thanks!

@saito-hideki saito-hideki force-pushed the issue/582 branch 2 times, most recently from 7504453 to f76f488 Compare October 30, 2024 07:58
@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/394e280f5a4641aeaec82d5bbb9af888

✔️ ansible-galaxy-importer SUCCESS in 5m 01s
✔️ build-ansible-collection SUCCESS in 5m 18s

@saito-hideki saito-hideki changed the title Changed the type of the forward option from str to bool [WIP] Changed the type of the forward option from str to bool Oct 30, 2024
@saito-hideki saito-hideki changed the title [WIP] Changed the type of the forward option from str to bool [WIP] Changed the type of the forward and masquerade options from str to bool Oct 30, 2024
@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/3e9a2c8c551240fb8529f645c8838535

✔️ ansible-galaxy-importer SUCCESS in 4m 06s
✔️ build-ansible-collection SUCCESS in 5m 29s

@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link
Contributor

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

✔️ ansible-galaxy-importer SUCCESS in 4m 29s
✔️ build-ansible-collection SUCCESS in 5m 33s

@saito-hideki saito-hideki force-pushed the issue/582 branch 2 times, most recently from 8b20cb0 to 96edc0d Compare October 31, 2024 06:49
@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link
Contributor

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

✔️ ansible-galaxy-importer SUCCESS in 4m 26s
✔️ build-ansible-collection SUCCESS in 5m 25s

@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/3533c79efbb4460dba9e158b27541e91

✔️ ansible-galaxy-importer SUCCESS in 4m 19s
✔️ build-ansible-collection SUCCESS in 5m 22s

@saito-hideki saito-hideki changed the title [WIP] Changed the type of the forward and masquerade options from str to bool Changed the type of the forward and masquerade options from str to bool Oct 31, 2024
@saito-hideki
Copy link
Collaborator Author

@maxamillion @Andersson007 I have fixed the PR and would appreciate it if you could review it again.

Andersson007
Andersson007 approved these changes Oct 31, 2024
View reviewed changes
Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saito-hideki LGTM, thanks!

maxamillion
maxamillion approved these changes Oct 31, 2024
View reviewed changes
Copy link
Collaborator

@maxamillion maxamillion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@saito-hideki saito-hideki added the mergeit Gate PR in Zuul CI label Oct 31, 2024
@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).
https://ansible.softwarefactory-project.io/zuul/buildset/832a670077534c9b97286b6121f0971a

✔️ ansible-galaxy-importer SUCCESS in 4m 45s
✔️ build-ansible-collection SUCCESS in 5m 27s

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 5eacaba into ansible-collections:main Oct 31, 2024
43 checks passed
@saito-hideki
Copy link
Collaborator Author

@Andersson007 @maxamillion, thanks for the review.
It has been merged. the main branch :)

@Andersson007
Copy link
Contributor

@saito-hideki thanks for the contribution!

@saito-hideki saito-hideki mentioned this pull request Nov 28, 2024
Merged
softwarefactory-project-zuul bot added a commit that referenced this pull request Dec 2, 2024
@softwarefactory-project-zuul
Merge pull request #598 from saito-hideki/issue/586
f632fad 
[Breaking Change] [firewalld] Change type of icmp_block_inversion option from str to bool

SUMMARY
Changed the type of icmp_block_inversion option from str to bool

Fixes #586

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

ansible.posix.firewalld

ADDITIONAL INFORMATION
Related  #582 and #584

Reviewed-by: Adam Miller <[email protected]>
Reviewed-by: Andrew Klychkov <[email protected]>
saito-hideki pushed a commit to saito-hideki/ansible.posix that referenced this pull request Dec 3, 2024
@softwarefactory-project-zuul @saito-hideki
Merge pull request ansible-collections#598 from saito-hideki/issue/586
ce72b7b 
[Breaking Change] [firewalld] Change type of icmp_block_inversion option from str to bool

SUMMARY
Changed the type of icmp_block_inversion option from str to bool

Fixes ansible-collections#586

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

ansible.posix.firewalld

ADDITIONAL INFORMATION
Related  ansible-collections#582 and ansible-collections#584

Reviewed-by: Adam Miller <[email protected]>
Reviewed-by: Andrew Klychkov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Andersson007 Andersson007 Andersson007 approved these changes

@maxamillion maxamillion maxamillion approved these changes

@Akasurde Akasurde Awaiting requested review from Akasurde

Assignees

@saito-hideki saito-hideki

Labels
mergeit Gate PR in Zuul CI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ansible.posix v 1.6.0 new forward option has wrong type
3 participants
@saito-hideki @maxamillion @Andersson007