Added ND 4.1 version support to the nd_backup and nd_backup_restore module (DCNE-452) #141
Conversation
sajagana
changed the title
sajagana
force-pushed
the
452_nd_backup
branch
2 times, most recently
from
db14bd9 to
f912c37
Compare
sajagana
requested review from
akinross,
anvitha-jain,
gmicol,
lhercot,
samiib and
shrsr
as code owners
…ackup_restore module
sajagana
changed the title
| if not file_location and state in ["backup", "download"]: | ||
| nd.fail_json("Parameter 'file_location' is required when state is 'backup|download' for ND versions < 3.2.1.") | ||
| nd_backup_before_3_2_1(module, nd, name, encryption_key, file_location, backup_key, state) | ||
| elif nd.version >= "3.2.1": |
| if len(encryption_key) < 8: | ||
| nd.fail_json("Please provide a minimum of 8 alphanumeric characters for the encryption key.") | ||
| elif not (any(char.isalpha() for char in encryption_key) and any(char.isdigit() for char in encryption_key) and encryption_key.isalnum()): | ||
| nd.fail_json("The encryption_key must contain at least one letter and one number, and have a minimum length of 8 characters.") |
There was a problem hiding this comment.
is ND not failing on these errors? I thought the agreement was made before to do the least amount of validation and let ND API handle it
| if file_location: | ||
| response = nd.request("{0}/{1}/actions/download".format(path, name), method="GET", data=None, output_format="raw") | ||
| write_file(module, file_location, to_bytes(response)) | ||
| elif module.check_mode: |
There was a problem hiding this comment.
also if nd.previous = nd.existing was set during query then elif state == "backup": could be combined with and not nd.existing: like you do for the download case
| nd.existing = backups.get("backups", []) | ||
|
|
||
| if state == "absent" and nd.existing: | ||
| nd.previous = nd.existing |
There was a problem hiding this comment.
Is it intended to do this here instead of already in the querying part?
| elif state == "download" and file_location and nd.existing: | ||
| if not module.check_mode: |
There was a problem hiding this comment.
why not combine this when you are already making a complicated elif
| elif state == "restore" and (not nd.existing or nd.existing.get("state") != "processing"): | ||
|
|
||
| if not module.check_mode: # Need to delete the imported file before starting the restore process | ||
| nd.request(import_path, method="DELETE") |
There was a problem hiding this comment.
Do we wait or verify that the file is deleted? How is ND handling this process? Could it have issues that the process is still running on ND, or you get response from API it is deleted?
|
|
||
| if not module.check_mode: | ||
| nd.request(import_path, method="POST", data=import_payload) | ||
| time.sleep(10) |
There was a problem hiding this comment.
why do we sleep here? should we do validation of task completion? Assume there is some delay on ND, what if it takes longer then 10s?
|
|
||
| # Operation not allowed if backup status is processing | ||
| elif state != "query" and nd.existing and nd.existing.get("state") == "processing": | ||
| nd.fail_json( |
There was a problem hiding this comment.
should we fail or provide a retry mechanism for this case?
|
|
||
| def nd_backup_restore_before_3_2_1(nd, name, encryption_key, file_location, restore_key, state, module): | ||
| if state == "restore" and not (name and encryption_key and file_location): | ||
| nd.fail_json("state is restore but all/one of the following are missing: name, encryption_key, file_location") |
There was a problem hiding this comment.
should we make explicit what is missing?
| if method in ("PUT", "DELETE", "PATCH"): | ||
| nd.existing = nd.previous = sanitize(nd.query_obj(path, ignore_not_found_error=True), ND_REST_KEYS_TO_SANITIZE) | ||
| if not ignore_previous_state: |
There was a problem hiding this comment.
combine conditional like you do in other conditionals?
fixes: DCNE-452