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.

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

Add optional timeouts for project managers #475

Merged
merged 4 commits into from
Nov 8, 2024

Conversation

gregorymacharia
Copy link
Contributor

On a previous incident occurring due to a disruption in npmjs.org, there were very long response times (~60s) that ultimately came back with 503 responses. This impacted Terrapin's latencies and availability. This change allows users to specify timeouts when they use OSSGadget's project managers when they call CreateProjectManager

Copy link
Contributor

@gfs gfs left a comment

Choose a reason for hiding this comment

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

Seems like a fine change to make, though not clear to me that the tests here are actually exercising the timeout functionality itself to confirm its working which should be possible by mocking the request as a sleep.

@gregorymacharia
Copy link
Contributor Author

Seems like a fine change to make, though not clear to me that the tests here are actually exercising the timeout functionality itself to confirm its working which should be possible by mocking the request as a sleep.

Makes sense I will add another commit to mock a delay and add to those tests

Copy link
Contributor

@gfs gfs left a comment

Choose a reason for hiding this comment

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

With the test to confirm timeout functionality I think this LGTM.

@pmalmsten pmalmsten merged commit 7b1a9fd into microsoft:main Nov 8, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants