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

fix: --max-backoff accepts seconds, not ms #588

Merged
merged 2 commits into from
Feb 7, 2025
Merged

Conversation

jrodewig
Copy link
Contributor

@jrodewig jrodewig commented Feb 5, 2025

Problem

The help text and tests for --max-backoff flag state/imply it accepts milliseconds.

The JS driver's corresponding maxBackoff client config option accepts values in seconds, not milliseconds. The value is converted from seconds to milliseconds here: https://github.com/fauna/fauna-js/blob/main/src/client.ts#L467-L468

Unless I missed it, the CLI does do any conversion of --max-backoff values so it should accept seconds.

Solution

Update the help text and tests to indicate --max-backoff accepts seconds.

Result

Accurate help and tests for the CLI.

Testing

npm run test:local is 🟢

@jrodewig jrodewig requested a review from a team as a code owner February 5, 2025 16:19
@jrodewig jrodewig requested a review from ptpaterson February 5, 2025 16:19
Copy link
Contributor

@ptpaterson ptpaterson left a comment

Choose a reason for hiding this comment

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

Yep! Thank you for catching!

@jrodewig jrodewig requested a review from a team February 5, 2025 16:32
Copy link
Contributor

@cleve-fauna cleve-fauna left a comment

Choose a reason for hiding this comment

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

Yikes, we have some in seconds and some in milliseconds? We need to fix that in the driver(s) and make them all milliseconds.

@jrodewig jrodewig merged commit 37fbe45 into main Feb 7, 2025
4 checks passed
@jrodewig jrodewig deleted the fix-max-backoff branch February 7, 2025 15:04
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