-
Notifications
You must be signed in to change notification settings - Fork 153
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
Display warning message for masquerade and icmp-block-inversion #254
Conversation
Closing and reopening for CI trigger |
Hi @Akasurde @Andersson007 [1] #249 (review) |
looking |
There was a problem hiding this 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.
…block-inversion * This PR is a part of #249 Signed-off-by: Hideki Saito <[email protected]>
Hi @Andersson007 |
@@ -0,0 +1,5 @@ | |||
--- | |||
minor_changes: |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@Akasurde @Andersson007 thanks guys! :) |
SUMMARY
Display warning message if the wrong parameter set to
masquerade
oricmp-block-inversion
It is a part of #249. Currently, the variable type of the above two parameters is
str
, but will bechanged
to bool in the future. As a starting point, this fix displays a warning message if a non-boolean value is specified.ISSUE TYPE
COMPONENT NAME
ADDITIONAL INFORMATION