Skip to content

-Adjusted CIMC proxy module to properly handle HTTP error codes and#48095

Merged
rallytime merged 5 commits intosaltstack:developfrom
spenceation:cimc-proxy-http-patch
Aug 13, 2018
Merged

-Adjusted CIMC proxy module to properly handle HTTP error codes and#48095
rallytime merged 5 commits intosaltstack:developfrom
spenceation:cimc-proxy-http-patch

Conversation

@spenceation
Copy link
Contributor

What does this PR do?

Adjusted CIMC proxy module to properly handle HTTP errors and timeouts.

What issues does this PR fix or reference?

Currently CIMC modules will display error messages when the CIMC proxy timeouts or receives a HTTP error code. This patch will apply the appropriate exception code during proxy module calls.

Previous Behavior

Error messages are displayed instead of device is not reachable.

New Behavior

Devices will now properly display not reachable messages when timeouts or HTTP error codes occur.

Tests written?

No

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

log.debug("Received error HTTP status code: {0}" .format(str(r['status'])))
logout(cookie)
raise salt.exceptions.CommandExecutionError(
"Did not receive a valid response from host.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe log what the error code we received was?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be logging the error code on line 157.

status=True,
headers=DETAILS['headers'])

if str(r['status']) not in ['200', '201', '202', '204']:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is identical to L156. Refactor for DRY?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change this to be included in a single function.

status=True,
headers=DETAILS['headers'])

if str(r['status']) not in ['200', '201', '202', '204']:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above for refactoring for DRY.

@rallytime
Copy link
Contributor

Hey @spenceation! I'm checking in here - did you get a chance to implement the feedback above?

@spenceation
Copy link
Contributor Author

Hey @rallytime. I've added the requested changes.

@rallytime rallytime requested a review from mirceaulinic July 19, 2018 19:53
Copy link
Contributor

@mirceaulinic mirceaulinic left a comment

Choose a reason for hiding this comment

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

Just a small suggestion, otherwise looks good to me.

if formatted_response_code not in ['200', '201', '202', '204']:
if cookie_to_logout:
logout(cookie_to_logout)
log.debug("Received error HTTP status code: {0}" .format(formatted_response_code))
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps log.error would be more appropriate?

Copy link
Contributor

Choose a reason for hiding this comment

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

@spenceation What do you think about this comment here?

@rallytime rallytime merged commit 10e4636 into saltstack:develop Aug 13, 2018
@spenceation spenceation deleted the cimc-proxy-http-patch branch September 7, 2018 12:10
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.

4 participants