-
Notifications
You must be signed in to change notification settings - Fork 1
snapshot scheduling changes #12
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
base: master
Are you sure you want to change the base?
Conversation
hpe3parclient/client.py
Outdated
|
|
||
| def check_response(self, resp): | ||
| for r in resp: | ||
| if 'error' in str.lower(r) or 'invalid' in str.lower(r) or 'the schedule format' in str.lower(r): |
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.
How does 'the schedule format' point to an error? Could you paste the exact command and the error output
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.
Please paste the entire command and its original output
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.
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.
Add another check comparing the complete string. The reason for that is that the substring we are trying to find has the potential to appear in other places as well. We can't always be sure if this will only occur in this scenario
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.
done
hpe3parclient/client.py
Outdated
| " '%(err_resp)s' ") % | ||
| {'err_resp': err_resp}) | ||
| raise exceptions.HTTPNotFound(reason=err) | ||
| except exceptions.HTTPNotFound as ex: |
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.
Since we are not making any HTTP REST call, this exception is not right.
We must have an SSH related exception wherever we are only making CLI call. Look at exception.py for an exception you can use, or else extend the SSHException to reflect the correct exception
This holds true everywhere we are generically throwing HTTPNotFound exception and no REST/HTTP calls are placed
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.
done
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.
Just replacing a generic SSHException will not help. We need to be more specific when raising exception.
Look at exceptions.py and create the exception you need by extending SSHExceptions.
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.
should i use SSHException? or should i create DeleteScheduleException class in exception.py.
As per code , if Command fails it will be due to sshexception only
No description provided.