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

Add new module to manage backup policies #373

Merged
merged 37 commits into from
Mar 4, 2021
Merged

Add new module to manage backup policies #373

merged 37 commits into from
Mar 4, 2021

Conversation

coleneubauer
Copy link
Contributor

SUMMARY

Addition of a module to manage azure recovery services vault policies. The goal of the module is to manage the creation and deletion of backup policies of existing recovery services vaults.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

azure_rm_backuppolicy

ADDITIONAL INFORMATION

This is the addition of a new module. It currently only supports daily and weekly policies for Azure VMs but design consideration was taken to ensure this is easily expandable and new additions would be backwards compatible by having the policy type and run frequency as parameters.

After running with the state: present flag the policy specified will either be created or updated if a matching one exists. Matching would be defined by the policy name, vault name, and resource group matching.

After running with the state: absent flag the policy specified would be deleted if present or no change would occur if the policy is not.

@Fred-sun
Copy link
Collaborator

@coleneubauer Thank you for submitting PR. You can check the module through the link below. Thank you very much!

link: https://docs.ansible.com/ansible/devel/dev_guide/testing_sanity.html#testing-sanity

@coleneubauer
Copy link
Contributor Author

@Fred-sun Thank you for providing the sanity test resource. I fixed the issues in the latest commit.

@Fred-sun
Copy link
Collaborator

Fred-sun commented Jan 6, 2021

@coleneubauer Please add dependence info to requirements-azure.txt (azure-mgmt-recoveryservicesbackup and azure-mgmt-recoveryservices)! Thank you very much!

@Fred-sun Fred-sun added medium_priority Medium priority new_module_pr Add new modules work in In trying to solve, or in working with contributors labels Jan 6, 2021
@coleneubauer
Copy link
Contributor Author

@Fred-sun Thank you for pointing out the missing dependencies. I updated the pr to include the dependencies in the requirements-azure.txt

Copy link
Collaborator

@Fred-sun Fred-sun left a comment

Choose a reason for hiding this comment

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

@coleneubasuer Please help to format the document, and update the name of the class parameter, to maintain the consistency with other module naming! Thank you very much!

plugins/modules/azure_rm_backuppolicy.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_backuppolicy.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_backuppolicy.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_backuppolicy.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_backuppolicy.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_backuppolicy.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_backuppolicy.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_backuppolicy.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_backuppolicy.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_backuppolicy.py Outdated Show resolved Hide resolved
@Fred-sun
Copy link
Collaborator

Fred-sun commented Jan 7, 2021

@coleneubauer Also,would you help add azure_rm_backuppolicy_info module and add the example related to azure_rm_backupolicy_info.py to main.yml. Thank you very much!

coleneubauer and others added 5 commits January 7, 2021 15:42
Update references to "required" params

Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>
Documentation suggestions

Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>
@coleneubauer
Copy link
Contributor Author

coleneubauer commented Jan 7, 2021

@Fred-sun I have made the suggested updates

@coleneubasuer Please help to format the document, and update the name of the class parameter, to maintain the consistency with other module naming! Thank you very much!

@coleneubauer
Copy link
Contributor Author

I can do this. Should this be in its own PR or tacked onto this one?

@coleneubauer Also,would you help add azure_rm_backuppolicy_info module and add the example related to azure_rm_backupolicy_info.py to main.yml. Thank you very much!

@Fred-sun
Copy link
Collaborator

can do this. Should this be in its own PR or tacked onto this one?

@coleneubauer Yes, so that we main.yml can manage this module and get information! Thank you very much!

@coleneubauer
Copy link
Contributor Author

coleneubauer commented Jan 15, 2021

@Fred-sun I've added the info module and tested it. Everything is working. With the sanity test for the info module, I'm getting an error message talking about missing parameters in the documentation that the parent object is using. These params are only defined in the AzureRMModuleBase class. It is not referenced in the info class. Do you know why this error is happening? It should be ready to merge otherwise. See below

ERROR: Found 2 validate-modules issue(s) which need to be resolved:
ERROR: plugins/modules/azure_rm_backuppolicy_info.py:0:0: required_if-requirements-unknown: required_if contains terms in requirements which are not part of argument_spec: log_path
ERROR: plugins/modules/azure_rm_backuppolicy_info.py:0:0: required_if-unknown-key: required_if must have its key log_mode in argument_spec

@coleneubauer
Copy link
Contributor Author

@Fred-sun Info module is added. Both modules pass sanity tests. Added the info module to the tests and ran some test playbooks of both modules. Let me know if there's anything else you need before this can be merged. Thanks!

Copy link
Collaborator

@Fred-sun Fred-sun left a comment

Choose a reason for hiding this comment

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

@coleneubauser Please distinguish the two names. When you have finished the change, I will push forward the PR merge. Thank you very much!

@coleneubauer
Copy link
Contributor Author

@coleneubauser Please distinguish the two names. When you have finished the change, I will push forward the PR merge. Thank you very much!

This has now been updated and should now be ready to merge. Thank you.

@Fred-sun Fred-sun added ready_for_review The PR has been modified and can be reviewed and merged and removed work in In trying to solve, or in working with contributors labels Feb 18, 2021
@haiyuazhang haiyuazhang merged commit 9dc678c into ansible-collections:dev Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium_priority Medium priority new_module_pr Add new modules ready_for_review The PR has been modified and can be reviewed and merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants