-
Notifications
You must be signed in to change notification settings - Fork 69
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
Api refactoring #56
Api refactoring #56
Conversation
Signed-off-by: Aurel Canciu <aurelcanciu@gmail.com>
Nice 👍 |
c20dc7c
to
60d669d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good -- but there's a couple of things to fix up:
- copypasta in a comment (see comment)
- the commit message for the third commit doesn't match what that commit does (it doesn't introduce the standard Condition type, which is done earlier)
This is an attempt to bring the api and controller logic closer to what the other controller components already have set as patterns. 1. Adopt the k8s standard Condition type. 2. Rename `ScanInterval` to `Interval` to be consistent with the `Interval` attribute other Spec types have defined, translating to reconciliation interval. This attribute is now required. 3. Add `ScanTime` attribute to the `ScanResult` type, enabling keeping track of the last successful scan execution. Use this value for scan frequency throttling. 4. Add optional `Timeout` attribute to allow custom scan timeout handling. The default value is equal to that of the `Interval` attr. Signed-off-by: Aurel Canciu <aurelcanciu@gmail.com>
60d669d
to
5eec08d
Compare
Too much history rewriting :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All makes sense; thanks @relu 🥬
This is an attempt to bring the api and controller logic closer to what
the other controller components already have set as patterns.
ScanInterval
toInterval
to be consistent with theInterval
attribute other Spec types have defined, translating toreconciliation interval. This attribute is now required.
ScanTime
attribute to theScanResult
type, enabling keepingtrack of the last successful scan execution. Use this value for scan
frequency throttling.
Timeout
attribute to allow custom scan timeouthandling. The default value is equal to that of the
Interval
attr.What is to be debated:
I proposed a different means of handling the reconciliation interval, while previously this was an optional value, it is not required. I think setting the interval should be a conscious decision made by the user considering the expectation of scan frequency and potential throttling imposed by the registry service provider. With this approach, we could recommend a default value while not imposing any restrictions aside from that of it being more than or equal to
1s
, the rest of the logic remains in place.A potential future design proposed by @squaremo, is to not enforce a mandatory value and rather scan as often as possible if no value is provided, while also providing an attribute that would set an upper bound for throttling purposes (at most frequent).