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.

Sign up for GitHub

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

Jump to bottom

Retry on Retry-After response header #2540

Merged
merged 3 commits into from
Jun 15, 2022
Merged

Retry on Retry-After response header #2540

merged 3 commits into from
Jun 15, 2022

Conversation

htmldoug
Copy link
Contributor

@htmldoug htmldoug commented Mar 3, 2022 •
edited
Loading

Partially addresses #2355 by implementing Retry-After support.

seconds, not dates

I'm only handling response headers of the form Reply-After: 60 seconds since that's what github actually returns. Non-numeric/date-based values like the one in the RFC will be ignored:

scala> "Wed, 21 Oct 2015 07:28:00 GMT".toIntOption
val res0: Option[Int] = None

@codecov
Copy link

codecov bot commented Mar 3, 2022 •
edited
Loading

Codecov Report

Merging #2540 (6ccc1ea) into main (34eaf50) will increase coverage by 0.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2540      +/-   ##
==========================================
+ Coverage   81.10%   81.11%   +0.01%     
==========================================
  Files         144      144              
  Lines        2535     2537       +2     
  Branches       41       47       +6     
==========================================
+ Hits         2056     2058       +2     
  Misses        479      479
Impacted Files Coverage Δ
...la/org/scalasteward/core/application/Context.scala 70.58% <0.00%> (ø)
...scalasteward/core/client/ClientConfiguration.scala 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34eaf50...6ccc1ea. Read the comment docs.

exoego
exoego reviewed Mar 4, 2022
View reviewed changes
Copy link
Contributor

@exoego exoego left a comment

Choose a reason for hiding this comment

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

Great !!
Could you add test to
https://github.com/scala-steward-org/scala-steward/blob/main/modules/core/src/test/scala/org/scalasteward/core/client/ClientConfigurationTest.scala

casmith
casmith reviewed Mar 4, 2022
View reviewed changes
modules/core/src/main/scala/org/scalasteward/core/client/ClientConfiguration.scala Outdated
maybeRetried.getOrElse(Resource.pure(response))
}

run(5) // arbitrary limit to avoid unexpected cloud provider costs
Copy link

Choose a reason for hiding this comment

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

Can this be made configurable?

@htmldoug htmldoug mentioned this pull request Mar 4, 2022
Open
@htmldoug
Copy link
Contributor Author

htmldoug commented Mar 4, 2022

@exoego, thanks for the review. Test added.

@htmldoug
Copy link
Contributor Author

htmldoug commented Apr 1, 2022

Bump.

@htmldoug
Copy link
Contributor Author

@exoego Looks like the only remaining coverage gap is in the dependency injection stuff that wasn't covered previously.

Can this be merged?

russellremple
russellremple approved these changes Apr 20, 2022
View reviewed changes
Copy link

@russellremple russellremple left a comment

Choose a reason for hiding this comment

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

lgtm

@htmldoug
Copy link
Contributor Author

@fthomas are you interested in this change, or should I close the PR?

@exoego
Copy link
Contributor

exoego commented Jun 15, 2022 •
edited
Loading

Sorry for long delay.
Though GitHub does not return Retry-After header for all requests types, I believe this is going to fix some cases which is annoying users.

Thanks for your effort.
I will make a new release soon.

@exoego exoego added the enhancement New feature or request label Jun 15, 2022
@exoego exoego merged commit 1564f07 into scala-steward-org:main Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@exoego exoego exoego left review comments

@casmith casmith casmith left review comments

@russellremple russellremple russellremple approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@htmldoug @exoego @casmith @russellremple