search

blackducksoftware / hub-rest-api-python

HUB REST API Python bindings
Apache License 2.0
89 stars 104 forks source link

Remove Client.HubSession retry method whitelist as per #235 #237

Closed OffBy0x01 closed 1 year ago

OffBy0x01 commented 1 year ago

As per #235, currently we only allow retries for GET requests, I can't see any reason why we would have limited it so.

By removing the field, the defaults will be used - https://urllib3.readthedocs.io/en/stable/reference/urllib3.util.html#urllib3.util.Retry.DEFAULT_ALLOWED_METHODS

Users can still provide their own session object to the client constructor if this change is not suitable for them.

gsnyder2007 commented 1 year ago

Makes sense to me (to remove the restriction).

OffBy0x01 commented 1 year ago

Not POST - these methods: Utilities - urllib3 1.26.14 documentation https://urllib3.readthedocs.io/en/stable/reference/urllib3.util.html#urllib3.util.Retry.DEFAULT_ALLOWED_METHODS

However, we only retry on server error (5XX) and too many requests (429) by default. The only 'dangerous retry' I could foresee would be something returning 400 - which is the Black Duck code for request exceeded 5 minutes

Which of these was of concern?

On Fri, 24 Feb 2023 at 02:54, Jack Cherng @.***> wrote:

Does this imply POST will be silently retried by default? That sounds dangerous.

— Reply to this email directly, view it on GitHub https://github.com/blackducksoftware/hub-rest-api-python/pull/237#issuecomment-1442725857, or unsubscribe https://github.com/notifications/unsubscribe-auth/AETT4XLKLNJZBWB5ZLF2BXLWZAPHRANCNFSM6AAAAAAUS7HESE . You are receiving this because you modified the open/close state.Message ID: @.*** com>

jfcherng commented 1 year ago

@OffBy0x01 POST is not allowed. I saw it wrong. I am totally fine with the current PR and thus I immediately deleted my previou comment before someone replies.