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.

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

Display warning message for masquerade and icmp-block-inversion #254

Merged
merged 1 commit into from
Sep 6, 2021
Merged

Display warning message for masquerade and icmp-block-inversion #254

merged 1 commit into from
Sep 6, 2021

Conversation

saito-hideki
Copy link
Collaborator

SUMMARY

Display warning message if the wrong parameter set to masquerade or icmp-block-inversion

It is a part of #249. Currently, the variable type of the above two parameters is str, but will be changed to bool in the future. As a starting point, this fix displays a warning message if a non-boolean value is specified.

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

@saito-hideki saito-hideki changed the title Display warning message for masquerade and icmp-block-inversion [WIP] Display warning message for masquerade and icmp-block-inversion Aug 24, 2021
@saito-hideki saito-hideki reopened this Aug 24, 2021
@saito-hideki
Copy link
Collaborator Author

Closing and reopening for CI trigger

@saito-hideki saito-hideki reopened this Aug 24, 2021
@saito-hideki saito-hideki changed the title [WIP] Display warning message for masquerade and icmp-block-inversion Display warning message for masquerade and icmp-block-inversion Aug 24, 2021
@saito-hideki saito-hideki marked this pull request as ready for review August 24, 2021 14:56
@saito-hideki
Copy link
Collaborator Author

Hi @Akasurde @Andersson007
I have added a warning message for future releases according to the following comments [1] and [2]. If it is possible, can you guys review this PR?

[1] #249 (review)
[2] #249 (comment)

@Andersson007
Copy link
Contributor

looking

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 thanks for the PR!
I would use the module.warn method here rather than warnings that can be missed in the returned json so that users will see them at least in a console / awx logs.

If it sounds sensible, don't rush to change the code. Let's see what @Akasurde thinks on my suggestions.

changelogs/fragments/254_variable_warnings.yml Outdated Show resolved Hide resolved
plugins/modules/firewalld.py Outdated Show resolved Hide resolved
plugins/modules/firewalld.py Outdated Show resolved Hide resolved
plugins/modules/firewalld.py Outdated Show resolved Hide resolved
plugins/modules/firewalld.py Outdated Show resolved Hide resolved
plugins/modules/firewalld.py Show resolved Hide resolved
…block-inversion

* This PR is a part of #249

Signed-off-by: Hideki Saito <saito@fgrep.org>
@saito-hideki saito-hideki changed the title Display warning message for masquerade and icmp-block-inversion [WIP] Display warning message for masquerade and icmp-block-inversion Sep 6, 2021
@saito-hideki saito-hideki changed the title [WIP] Display warning message for masquerade and icmp-block-inversion Display warning message for masquerade and icmp-block-inversion Sep 6, 2021
@saito-hideki
Copy link
Collaborator Author

Hi @Andersson007
Thank you for your advice. As you pointed out, I think module.warn is more appropriate to address this case. So I have fixed this PR based on your suggestion.
Thanks again! :)

@@ -0,0 +1,5 @@
---
minor_changes:
Copy link
Member

Choose a reason for hiding this comment

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

I think this is breaking change. What do you think @Andersson007 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Akasurde i think the warning itself isn't, so i believe that's fine.
If the type change wasn't announced yet, we should add another fragment under major_changes: to announce that the change is coming soon.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I think then we can merge this #249 after this PR is merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah

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.

LGTM

@Akasurde Akasurde added the gate Gate PR in Zuul CI (Obsolete: Please set "mergeit" instead of "gate") label Sep 6, 2021
Copy link
Contributor

@ansible-zuul ansible-zuul bot 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
Copy link
Collaborator Author

@Akasurde @Andersson007 thanks guys! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gate Gate PR in Zuul CI (Obsolete: Please set "mergeit" instead of "gate")
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants