feat(remote): pass retryBackoff and retryPredicate options to transport

[aslafy-z]

• Pass options.retryBackoff and options.retryPredicate to new transport.

Fixes #1692

[imjasonh]

@jonjohnsonjr aside from the 429 change, do you see a reason we shouldn't retry with the custom semantics when we're also injecting a custom transport?

[jonjohnsonjr]

I had these be intentionally separate because if you want to control the transport retry behavior, you should be using remote.WithTransport.

[aslafy-z]

@jonjohnsonjr if so, I don't get what are all these options for..

[github-actions]

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Keep fresh with the 'lifecycle/frozen' label.

[aslafy-z]

This is still desired. @jonjohnsonjr is this still something you don't want for go-containerregistry?

[block-danm]

@jonjohnsonjr Could you expand on

if you want to control the transport retry behavior, you should be using remote.WithTransport.

please?

There are three settings that control retry behavior - status codes, predicate, and backoff - and currently all three are set in different ways:

• the status codes that trigger a retry attempt are set from o.retryStatusCodes, so they account for any settings provided with config.WithRetryStatusCodes();
• the retry predicate is always set to the defaultRetryPredicate defined in options.go;
• the backoff isn't explicitly set in makeOptions(), so it falls back to the default defined in retry.go.

This inconsistency is quite unexpected and confusing, and this PR would immediately resolve that confusion.

I am also very curious about @aslafy-z 's question - if config.WithRetryBackoff() does not affect the retry backoff used by the actual transport, what is it for?

[aslafy-z]

@imjasonh @jonjohnsonjr Can you please reconsider this PR? Thank you

[github-actions]

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Keep fresh with the 'lifecycle/frozen' label.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.88%. Comparing base (8b3c303) to head (f6da36f).
Report is 5 commits behind head on main.

❗ Current head f6da36f differs from pull request most recent head 10c7e61

Please upload reports for the commit 10c7e61 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1628      +/-   ##
==========================================
+ Coverage   71.67%   71.88%   +0.21%     
==========================================
  Files         123      122       -1     
  Lines        9935     9832     -103     
==========================================
- Hits         7121     7068      -53     
+ Misses       2115     2081      -34     
+ Partials      699      683      -16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[aslafy-z]

This PR is still desired.

[github-actions]

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Keep fresh with the 'lifecycle/frozen' label.

[aslafy-z]

I just rebased and fixed conflicts to this PR. Now that #1635 is merged, can we merge that one? Thank you

[hayk99]

is this going to be merged soon?

[aslafy-z]

I would love to see this merged..