Retention period validation

[chlunde]

Hi, it's tempting for someone to use the "d" suffix in retention period, but that's not valid. It will prevent the operator from working as it cannot deserialize TemporalNamespace into Go objects.

With

retentionPeriod: 30d

we got the following error:

Failed to watch *v1beta1.TemporalNamespace: failed to list *v1beta1.TemporalNamespace: time: unknown unit "d" in duration "30d"

This is because it is parsed by https://pkg.go.dev/time#ParseDuration

I tried to delete a temporal UI Deployment, and it was not recreated.

We've had the same issue in an in-house controller. We added the following annotations there:

    // Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h".
    // Format: 8h30m0s
    //+optional
    //+kubebuilder:validation:Type=string
    //+kubebuilder:validation:Pattern=^(\d+h)?(\d+m)?(\d+s)?$
    //+kubebuilder:validation:MinLength=2

[alexandrevilain]

Hi @chlunde !

Thanks for the issue. I understand your point but I'm pretty aligned with the Go team vision on the absence of the "day" in duration: https://github.com/golang/go/issues/11473#issuecomment-116930849

I don't think it's a good idea to start developing something around the duration to support the day parameter :)

Isn't 720h enough for you ? :)

[chlunde]

I just intended to report that we should probably add some comments like above here: https://github.com/alexandrevilain/temporal-operator/blob/main/api/v1beta1/temporalnamespace_types.go#L44-L45

    // RetentionPeriod to apply on closed workflow executions.
    // Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h".
    // Format: 8h30m0s
    //+optional
    //+kubebuilder:validation:Type=string
    //+kubebuilder:validation:Pattern=^(\d+h)?(\d+m)?(\d+s)?$
    //+kubebuilder:validation:MinLength=2
    RetentionPeriod *metav1.Duration `json:"retentionPeriod"`

So we don't get invalid values into the api server, because it stops the whole operator is seems (although I would only expect namespace reconcilation to stop)

[alexandrevilain]

Good point! Are you interested on contributing ?

it stops the whole operator is seems

What do you mean ? An issue on the NamespaceController stops the whole operator ?

[chlunde]

@alexandrevilain I was in a hurry so I didn't properly debug it, but I had added the prometheus annotation setting for TemporalCluster and deleted the Deployment for cluster-ui and expected the operator to re-create it or apply the change for prometheus. That's why I checked the logs and discovered this issue. At that point I fixed the TemporalNamespace retentionPeriod and the operator then fixed the TemporalCluster (annotation and missing deployment).

I can probably test and create a PR for the schema tomorrow.