@gnaponie @lholecek can you PTAL?

The retry has to be introduced also in _retrieve_yaml_remote_rule_web.
At the moment the "git archive" mode is not used (internally), due to a bug -- and it will never be used in Fedora. So we need to add the logic to both mechanisms.

Is the timeout set somewhere?

The retry has to be introduced also in _retrieve_yaml_remote_rule_web.
At the moment the "git archive" mode is not used (internally), due to a bug -- and it will never be used in Fedora. So we need to add the logic to both mechanisms.

@yashn handles this in _retrieve_yaml_remote_rule_web by configuring the Python requests session to retry on connection errors.

It's unclear how much value there is in retrying the git archive mode, but I suppose some flaky network issue could prevent that from working. So I don't personally have a problem with it.

We don't actually supply a timeout to communicate, so this exception will never get raised. If this retry is implemented, perhaps it makes sense for it to retry on a different failure.

We don't actually supply a timeout to communicate, so this exception will never get raised. If this retry is implemented, perhaps it makes sense for it to retry on a different failure.

Nice catch! I added a timeout value and now it makes sure it kills the old process before starting a new one.

How about moving this to separate submodule request_session.py containing:

session = _get_requests_session()

BTW, remove the unused argument.

I think you can remove the retry decorator from utils submodule.

Can you also use REQUESTS_TIMEOUT variable and remove its redundant uses?

Actually, apart from REQUESTS_TIMEOUT, there are also options RETRY_TIMEOUT and RETRY_INTERVAL, this is very confusing.

How about moving this to separate submodule request_session.py containing:
session = _get_requests_session()

imho, I do not think we should have a separate submodule for this. I think it fits just fine in utils.py.

BTW, remove the unused argument.

I had missed this. Thanks for pointing it out. :)

Actually, apart from REQUESTS_TIMEOUT, there are also options RETRY_TIMEOUT and RETRY_INTERVAL, this is very confusing.

Sure, I will remove RETRY_TIMEOUT and RETRY_INTERVAL, but the usage of REQUESTS_TIMEOUT still seems relevant. Thoughts?

I did remove the retry decorator before but I think I missed it when I rebased it. Fixing it now :)

imho, I do not think we should have a separate submodule for this. I think it fits just fine in utils.py.

Problems with submodules in a single file with generic name (like utils.py or common.py) is that they can grow to a really huge size which contains lot of unrelated stuff and lots of imports which is difficult to read and maintain.

imho, I do not think we should have a separate submodule for this. I think it fits just fine in utils.py.

Problems with submodules in a single file with generic name (like utils.py or common.py) is that they can grow to a really huge size which contains lot of unrelated stuff and lots of imports which is difficult to read and maintain.

Agreed, but maybe we should move it to separate file in-case we end up with a huge file? And at the same time, I don't feel it is required to have a separate submodule for a single method. But I would be happy to do it if you feel strongly about it. Thoughts? :)

I don't feel it is required to have a separate submodule for a single method.

Not required, but preferable. In small files it's easier to find stuff, make sense of the imports and see if something is unused.

But I would be happy to do it if you feel strongly about it.

I'm OK with merging this as is, but I have strong feeling that we will have to refactor the utils.py in future.

+1

made the change. :)

Just one thing I would like to bring up, the functional tests seem to fail when I use vagrant. The ones that interact with WaiverDB with requests.exceptions.HTTPError: 401 Client Error: UNAUTHORIZED for url: http://localhost:5004/api/v1.0/waivers/. I guess this is because of the access control we have recently added to waiverdb. Can I please ask if that's right or am I missing something? Anything I can do to make it work using vagrant?

+1

Not sure what's wrong with vagrant (possibly the dependency list needs an update); I use only docker-compose.

@yashn they fail I assume due to the recent changes we made in waiverdb. I think we are good to merge this PR. I'm gonna check now how to solve the vagrant and we can make eventually another PR.

:thumbsup: LGTM!

Commit 54bf42d5 fixes this pull-request