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: allow disabling smart retries [gh-943] #944

Merged
merged 3 commits into from
Mar 25, 2024

Conversation

clample
Copy link
Collaborator

@clample clample commented Mar 25, 2024

I hereby confirm that I followed the code guidelines found at engineering guidelines

Affected Components

  • CLI
  • Create CLI
  • Test
  • Docs
  • Examples
  • Other

Notes for the Reviewer

Resolves #943

Currently it's not straightforward to disable smart retries. Setting retryStrategy: null for a check gives a type error when using TypeScript (type 'null' is not assignable to type 'RetryStrategy | undefined'). When using JS, the backend Joi default of doubleCheck: true shows in the UI as a linear retry strategy. Currently to disable all retries you need to set both retryStrategy: null and doubleCheck: false.

This PR makes it simpler to disable smart retries. To disable smart retries, the user can just set retryStrategy: null on a check or a group - there's no need to also set doubleCheck: false. When a user sets retryStrategy: null and doesn't set doubleCheck, the CLI will set doubleCheck: false to avoid the Joi default.

This approach of fixing the issue on the CLI side should minimize the breaking changes for users. The only breaking change is that any users that had retryStrategy: null and that weren't setting explicitly setting doubleCheck will have their retries disabled when upgrading to the new CLI version (since the doubleCheck: true default is removed). This shouldn't really have an impact, though.

The PR also adds a RetryStrategyBuilder.noRetries() method to make it more clear/explicit how to disable retries.

@clample clample added the build Issue regarding building and packaging label Mar 25, 2024

This comment has been minimized.

@clample clample requested a review from umutuzgur March 25, 2024 11:43
/**
* No retries are performed.
*/
static noRetries (): null {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't think of a good name that matches the <something>Strategy pattern that we have.

@clample clample added build Issue regarding building and packaging and removed build Issue regarding building and packaging labels Mar 25, 2024
Copy link

🎉 Experimental release successfully published on npm

npm install [email protected]

Copy link
Member

@umutuzgur umutuzgur left a comment

Choose a reason for hiding this comment

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

LGTM

@clample clample merged commit 4fcc819 into main Mar 25, 2024
4 checks passed
@clample clample deleted the chrislample/gh-943/allow-disabling-retry-strategies branch March 25, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issue regarding building and packaging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: default retry strategy is added without having any strategy defined
2 participants