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

Specify server timeout #2246

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

just-an-engineer
Copy link

This should fix issue #2237, where in the case a compilation is still ongoing when a shutdown request is sent, and it's a larger/complex codebase that takes longer than the default 10 seconds, the user can now specify it in their config file.

However, I'd like to also propose a breaking change for config files, moving server_startup_timeout_ms and server_shutdown_timeout_ms to a specific timing struct. I left the code in there to do this, just commented out. I'd like to make this change next time we bump up a major version.
Although I can remove the comments to clean up the code until then. But what's the best practice for marking a milestone? Should I raise an issue?

@just-an-engineer just-an-engineer marked this pull request as draft August 12, 2024 18:43
@just-an-engineer just-an-engineer marked this pull request as ready for review August 12, 2024 18:49
@just-an-engineer just-an-engineer marked this pull request as draft August 12, 2024 19:03
@just-an-engineer just-an-engineer marked this pull request as ready for review August 13, 2024 15:23
@just-an-engineer
Copy link
Author

If a breaking change will occur in 0.9, like @Xuanwo had mentioned in PR #2235, talking about the issue to replace rouille with axum, then I can go ahead and change this PR to include that breaking change for the config file, and we can merge this in with 0.9. If that will occur. Otherwise, it's ready to go, outside of the comment I previously wrote

@sylvestre
Copy link
Collaborator

seems that rustfmt needs to be run
also, could you please document this into the readme file? thanks

@codecov-commenter
Copy link

codecov-commenter commented Aug 31, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 24 lines in your changes missing coverage. Please review.

Project coverage is 51.67%. Comparing base (0cc0c62) to head (554425c).
Report is 72 commits behind head on main.

Files with missing lines Patch % Lines
src/config.rs 52.50% 2 Missing and 17 partials ⚠️
src/server.rs 0.00% 0 Missing and 3 partials ⚠️
tests/oauth.rs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2246       +/-   ##
===========================================
+ Coverage   30.91%   51.67%   +20.76%     
===========================================
  Files          53       54        +1     
  Lines       20112    20751      +639     
  Branches     9755     9795       +40     
===========================================
+ Hits         6217    10724     +4507     
- Misses       7922     8026      +104     
+ Partials     5973     2001     -3972     

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

Copy link
Collaborator

@sylvestre sylvestre left a comment

Choose a reason for hiding this comment

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

seems that rustfmt needs to be run
also, could you please document this into the readme file? thanks

@just-an-engineer
Copy link
Author

@sylvestre, could I make a flag doc or something along those lines? I feel like the readme is getting too long. Perhaps cargo doc? I haven't used it yet, though, so I'm not too sure. I just think that's a better alternative to add this option to over just the readme

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