Switch ints to time.Duration #427
Conversation
|
not sure if we are allowed to do reviews when not core team, we would need to update the documentation on this in the user guide i believe. I am excited for this one. ;) |
| @@ -40,7 +41,7 @@ var ( | |||
| // Consult the API reference for the Group, Version and Kind of a resource: https://kubernetes.io/docs/reference/ | |||
| // namespace is the Namespace to watch for the resource | |||
| // TODO: support opts for specifying label selector | |||
| func Watch(apiVersion, kind, namespace string, resyncPeriod int, opts ...watchOption) { | |||
| func Watch(apiVersion, kind, namespace string, resyncPeriod time.Duration, opts ...watchOption) { | |||
There was a problem hiding this comment.
Can we update the godoc for this change as well?
|
@CSdread feel free to code review any existing PRs. |
pkg/generator/templates.go
Outdated
| @@ -159,7 +160,7 @@ func main() { | |||
| if err != nil { | |||
| logrus.Fatalf("failed to get watch namespace: %v", err) | |||
| } | |||
| resyncPeriod := 5 | |||
| resyncPeriod := time.Duration(5) | |||
There was a problem hiding this comment.
Shouldn't' this be a default of 5 seconds:
resyncPeriod := time.Duration(5) * time.SecondOtherwise simply casting it to time.Duration means this is 5ns.
Duration is just an int64 nanosecond count.
https://golang.org/pkg/time/#Duration
pkg/sdk/informer.go
Outdated
| @@ -56,7 +56,7 @@ func NewInformer(resourcePluralName, namespace string, resourceClient dynamic.Re | |||
| numWorkers: n, | |||
| } | |||
|
|
|||
| resyncDuration := time.Duration(resyncPeriod) * time.Second | |||
| resyncDuration := resyncPeriod * time.Second | |||
There was a problem hiding this comment.
No need to multiply by time.Second here.
The user is allowed to pass in any Duration to sdk.Watch().
pkg/sdk/api.go
Outdated
| @@ -35,12 +36,12 @@ var ( | |||
| // - Pods have Group "Core" and Version "v1" giving the APIVersion "v1" | |||
| // - The custom resource Memcached might have Group "cache.example.com" and Version "v1alpha1" giving the APIVersion "cache.example.com/v1alpha1" | |||
| // kind is the Kind of the resource, e.g "Pod" for pods | |||
| // resyncPeriod is the time period in seconds for how often an event with the latest resource version will be sent to the handler, even if there is no change. | |||
| // resyncPeriod is the time period in ms for how often an event with the latest resource version will be sent to the handler, even if there is no change. | |||
There was a problem hiding this comment.
resyncPeriod is the time period in time.Duration (int64 nanoseconds count)...
|
LGTM |
|
lgtm |
|
@theishshah We forgot to update the user-guide for this. In the watch the CR section we need to change the argument type to duration for the resync period. sdk.Watch("cache.example.com/v1alpha1", "Memcached", "default", 5)to: Can you put up a PR for that. |
Issue #345