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

U-4230 Make request_timeout for monitors docs more consistent and easier to parse #133

Merged
merged 2 commits into from
Jan 16, 2025

Conversation

PetrHeinz
Copy link
Member

The original format (#131) was running into issues when our internal checks tried to parse it, not understanding the arguments below it correctly.

This was it should be easy to parse, and maybe a bit easier to read as well.

Before:

image

After:

image

@PetrHeinz PetrHeinz requested a review from iatanas0v January 16, 2025 11:00
Copy link
Contributor

@iatanas0v iatanas0v 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 making it extra readable @PetrHeinz 🙏

Description: "How long to wait before timing out the request?\n" +
" - For Server and Port monitors (types `ping`, `tcp`, `udp`, `smtp`, `pop`, `imap` and `dns`) the timeout is specified in *milliseconds*. Valid options: 500, 1000, 2000, 3000, 5000.\n" +
" - For Playwright monitors (type `playwright`), this determines the Playwright scenario timeout instead in *seconds*. Valid options: 15, 30, 45, 60.\n" +
" - For all other monitors, the timeout is specified in *seconds*. Valid options: 2, 3, 5, 10, 15, 30, 45, 60.\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

The trailing newline is the actual fix, correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the trailing new line wasn't even necessary, the issue there was I believe the missing dashes, making the list end early. I think the trailing whitespace might even get trimmed when running make gen - not sure exactly, tbh.

I added the newline just so it's easy to copy paste the lines if more cases are added in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Thanks for explaining 🙏

@PetrHeinz PetrHeinz merged commit 66a4ed3 into master Jan 16, 2025
21 checks passed
@PetrHeinz PetrHeinz deleted the ph/docs-update branch January 16, 2025 13:23
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.

2 participants