Skip to content
This repository was archived by the owner on Oct 14, 2024. It is now read-only.

Shem/max redirect state update #258

Merged
merged 9 commits into from
Jan 22, 2024
Merged

Conversation

shemogumbe
Copy link
Contributor

@shemogumbe shemogumbe commented Dec 4, 2023

fixes #246

Copy link

sonarqubecloud bot commented Dec 4, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@andrueastman andrueastman left a comment

Choose a reason for hiding this comment

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

We probably need to check on the failing CI due to linting errors as well

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

We have the same issue for the retry handler and I think we should take that opportunity to fix all the occurrences that pattern.

current_options.max_delay -= (end_time - start_time)

@samwelkanda
Copy link
Contributor

Thanks for the changes @shemogumbe. Remember to add a CHANGELOG entry and bump package version once done.

@tpcgold
Copy link

tpcgold commented Jan 18, 2024

@baywet blocking a PR for the severe "max_redirect" bug:
#246
is annoying. you could do another PR for retry_handler.

a lot of people (including me) are waiting for this fix.

@baywet
Copy link
Member

baywet commented Jan 18, 2024

Thanks for the feedback @tpcgold
@isvargasmsft to weight in on whether you'd like to fast-track only the redirect fix, or fix the same problematic pattern across all handlers (retry, etc..)?

@isvargasmsft
Copy link

Thanks for the feedback @tpcgold @isvargasmsft to weight in on whether you'd like to fast-track only the redirect fix, or fix the same problematic pattern across all handlers (retry, etc..)?

Let's unblock users with this fix now but make sure we add the issue to the backlog to be resolved this quarter.

baywet
baywet previously approved these changes Jan 18, 2024
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thanks for the guidance @isvargasmsft
@shemogumbe can you also add a changelog entry please?

@baywet
Copy link
Member

baywet commented Jan 18, 2024

Created #281 to follow up

Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@samwelkanda samwelkanda merged commit 8b265e8 into main Jan 22, 2024
@samwelkanda samwelkanda deleted the shem/max_redirect_state_update branch January 22, 2024 16:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RedirectHandler maintains max_redirect state across requests
6 participants