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

Refactoring nxos_save_config #39

Merged

Conversation

GGabriele
Copy link
Contributor

Here I changed parsed_data_from_device a little bit to handle errors there. Let me know if this works for you :)

@jedelman8
Copy link
Owner

Just some thoughts...

splitted_save = save.split('\n')

Do we actually need to do the split here?

The following conditional should work either way, but rather check in each, check save. BTW, maybe we should call that var save_response to be clearer instead of save

    if '100%' in each or 'copy complete' in each.lower():

Anything else we can do to improve the code block below? At a minimum, I'm thinking add the response when the module fails such as ` module.fail_json(msg=error, response=save_response)

if complete:
    result = 'successful'
else:
    error = 'error2: could not validate save'

if error is not None:
    module.fail_json(msg=error)
else:
    return (result, changed)

What about this?

if complete:
    result = 'successful'
    return (result, changed)
else:
    error = 'error: could not validate save'
    module.fail_json(msg=error, response=save_response)

Last thing for now...change the save key in the results dictionary being returned. We need to be more descriptive with variables...how about calling it status?

@GGabriele
Copy link
Contributor Author

Yeah, you're right man. I've just made the chages you suggested.

@jedelman8
Copy link
Owner

Can you verify the error message is correct?

           msg = ('invalid format for path.  Requires ":" ' +
                        'Example- bootflash:config.cfg' +
                        'or bootflash:/configs/test.cfg')

Should the / be there in front of "configs"?

Also, why do the comments in the test playbook say something about the file existing or not existing first to be successful?

Does the configs directory (or any dir) need to exist before using this module? If so, can you add that to the notes section?

@GGabriele
Copy link
Contributor Author

Yes, if I try to overwrite an existing dir I receive this error:

{"error": "NX-OS CLI Configuration Error\nPermission denied. Cannot overwrite existing file.. Permission denied", "failed": true}

Now I fix the doc.

jedelman8 added a commit that referenced this pull request Dec 29, 2015
@jedelman8 jedelman8 merged commit 6d559cb into jedelman8:modules-no-pycsco-utils Dec 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants