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

Check functions do not need to know the interval #67

Open
iainlane opened this issue Jul 14, 2023 · 0 comments
Open

Check functions do not need to know the interval #67

iainlane opened this issue Jul 14, 2023 · 0 comments
Labels
good-first-issue Good issue for new contributors to the project: not too simple, and would have decent impact

Comments

@iainlane
Copy link
Member

@julienduchesne raised this in a comment on one of our recent PRs.

It's weird that the Check interface has the recheck interval as a parameter on its function

Check(ctx context.Context, recheckInterval time.Duration) error

The check functions are not actually doing the waiting themselves. That happens in RunUntilCancelledOrTimeout. This is where the ticker is created, and it shouldn't need to pass the interval further on.

We only really do this so we can log that we are rechecking in period.

We can either move that particular log statement up a few levels, or restructure how we pass around loggers, and make sure there's always one available that has sensible fields set on it. Either way the signature of Check should be Check(ctx context.Context) error.

@iainlane iainlane added the good-first-issue Good issue for new contributors to the project: not too simple, and would have decent impact label Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-first-issue Good issue for new contributors to the project: not too simple, and would have decent impact
Projects
None yet
Development

No branches or pull requests

1 participant