Add get config timeout - Githubissues

luiscoms commented 4 years ago

Any considerations?

luiscoms commented 4 years ago

Any considerations?

amenezes commented 4 years ago

Sorry for delay @luiscoms I think that the timeout support it's a great idea. Just one thing that maybe can be used it's **kwargs over timeout property this would make the request more flexible allowing, for example, set redirection, headers and another options provided by requests.

Another question that should be observed is that headers for a request, for example, are currently passed in get_config method, maybe change this for a kwargs be enough.

I also find it interesting and necessary to include some tests covering success and failure scenarios.

luiscoms commented 4 years ago

I just updated the pull request with your suggestions

luiscoms commented 4 years ago

hi @amenezes did you viewed this?

amenezes commented 4 years ago

Sorry for delay @luiscoms. I'm still validate, maybe until end of this week I make the merge.

amenezes commented 4 years ago

@luiscoms,

Finally I finished the code analysis :raised_hands:

Some points that need changes:

with **kwargs the timeout it's no more necessary. If you wish set timeout or headers, for example, just put them on get_config. As the example below:

...

myconfig.get_config(timeout=0.10, headers={"Content-Type": "application/json"})

All fields will be passed for: https://github.com/amenezes/config-client/blob/f8cffcb7897f5f463ef1b0225b0537ba4c9c5b3f/config/spring.py#L104

So then these lines can be removed:

with use of raise_for_status, we can rewrite the methods get_config and _request_config for something like:

...

def get_config(self, **kwargs) -> None:
        response = self._request_config(**kwargs)
        self._config = response.json()

def _request_config(self, **kwargs) -> requests.Response:
        try:
            response = requests.get(self.url, **kwargs)
            response.raise_for_status()
        except requests.exceptions.HTTPError:
            raise RequestFailedException(
                "Failed to request the configurations. HTTP Response"
                f"url={self.url}, code={response.status_code})"
            )
        except Exception:
            logging.error("Failed to establish connection with ConfigServer.")
            if self.fail_fast:
                logging.info("fail_fast enabled. Terminating process.")
                raise SystemExit(1)
            raise ConnectionError("fail_fast disabled.")
        return response

I think that's it.

If you find some problem with tests contact me that I share with you some improvements that will be included with this PR.

luiscoms commented 4 years ago

Hi @amenezes the propose of timeout environment variable is to set the default timeout even if we do not pass parameter, without change the get_config call on the code.

with **kwargs the timeout it's no more necessary. If you wish set timeout or headers, for example, just put them on get_config. As the example below:
...

myconfig.get_config(timeout=0.10, headers={"Content-Type": "application/json"})

amenezes commented 4 years ago

@luiscoms,

I don't know if enforce this timeout for all use case is the best option. Could you use timeout as environment variable for your needs but passing the value in the get_config. For example:

myclient.get_config(timeout=float(os.getenv('TIMEOUT', 5)))

Wouldn't that break the cohesion of the class?

luiscoms commented 4 years ago

Hi @amenezes I done above changes

amenezes / config-client

Add get config timeout #29