-
Notifications
You must be signed in to change notification settings - Fork 334
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
New Module : azure_rm_firewallpolicy #705
New Module : azure_rm_firewallpolicy #705
Conversation
Integration test results -
|
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.
small change request!
self.threat_intel_mode.lower() != results['threat_intel_mode'].lower(): | ||
changed = True | ||
results['threat_intel_mode'] = self.threat_intel_mode | ||
if self.threat_intel_whitelist: |
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.
The processing logic here doesn't make sense. The recommendation is to compare the current parameter with the obtained result. If the resource does not exist, the parameter is the current configuration and is created directly. If the resource exists and the obtained configuration information contains the current parameters, then no update is made and idempotency holds. If the resource exists, the obtained configuration information does not contain the current configuration parameters. Will update and add new configurations. Thank you very much!
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.
You can also refer to the update logic of tags. Thank you very much!
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.
Sure @Fred-sun Thank you for suggestions! Let me work on it.
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.
Hi @Fred-sun, I have updated the logic for self.threat_intel_whitelist as suggested. Could you please check it once again? Thank you very much!
@aparna-patil self. threat_intel_whitelist update logic also needs your help to change. Thank you very much!
|
Hi @Fred-sun, I have made the suggested changes. Could you please review this PR once again? |
@aparna-patil The update logic is not reasonable. If the current resource contains the configuration in the script, do not update it. If some parameters in the configuration do not belong to the current configuration, add new parameters. Thank you very much! |
Hi @Fred-sun Thank you for reviewing! Just wanted to check if update logic needs to be revised for all the parameters or self.threat_intel_whitelist param only? Also, If you could share the reference module which I can refer to for correct implementation of this update logic would really help me. Thank you very much! |
@aparna-patil You can refer to module -- azure_rm_virtualnetwork.py. |
@aparna-patil Thank you for your contribution. The dependencies related to network have been updated. Please help rebase this PR and apply the latest SDK. Thank you very much! |
kindly ping! |
Hi @Fred-sun , Sure will work on the changes using new SDK. Thanks! |
@aparna-patil Thanks for your reply, you can rebase PR and retest below, as the new SDK functions have different names from the old SDK functions. Thank you very much! |
@aparna-patil If you don't have time, you can authorize me to change this PR together, thank you! |
Hi @Fred-sun Thank you for helping! I am unable to take out time to work on this PR due to some other commitments. I have provided the access to this PR. Could you please check once? Thank you once again! |
@aparna-patil I am sorry that I do not have your authorization to modify this change together. Thank you very much! |
Hi @Fred-sun I have added you as a collaborator on my repository. Could you please check now if you have the write access to the branch? Thank you very much! |
LGTM |
SUMMARY
This module supports following features -
ISSUE TYPE
COMPONENT NAME
azure_rm_firewallpolicy
azure_rm_firewallpolicy_info
ADDITIONAL INFORMATION