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

added support to set timeout for TAXII requests #41

Merged
merged 2 commits into from
Jun 19, 2017

Conversation

joarleymoraes
Copy link
Contributor

@joarleymoraes joarleymoraes commented May 5, 2017

Fixing #40


This change is Reviewable

@@ -161,6 +162,7 @@ def _execute_request(self, request, uri=None, service_type=None):
services by ``service_type``.
'''

timeout = self.timeout
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to have a separate local variable

@@ -47,6 +47,7 @@ def __init__(self, host=None, discovery_path=None, port=None,

self.jwt_token = None
self.headers = headers or {}
self.timeout = None
Copy link
Contributor

Choose a reason for hiding this comment

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

please add this to constructor arguments

@traut
Copy link
Contributor

traut commented May 9, 2017

thank you for the pull request! Timeout is something that should've been added long time ago.

Before I merge it, please adjust your code a bit:

  • make sure flake8 does not complain about cabby and tests codebases. It shows 2 issues now
  • please refactor provided test to use mocked URL with time.sleep() call inside. You can use dynamic body generation in httpretty for that. Some users will have network setup that returns an error for http://httpbin.org:81/ instantly, making the test fail.
  • see my comments on the source

@traut
Copy link
Contributor

traut commented May 9, 2017

also, please sync your branch with master. I removed ipdb dependency and that can help with travis-ci build failure

@joarleymoraes
Copy link
Contributor Author

Ok, will fix the issues.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 85.5% when pulling bf53cf7 on joarleymoraes:master into cb5c1cc on EclecticIQ:master.

@traut traut merged commit 11a1d42 into eclecticiq:master Jun 19, 2017
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

3 participants