[niyomukiza-mechack]

What was changed

Adds the deletion policy field and the decision logic for deleting a Temporal Cloud namespace.

• pkg/deletion: shared logic for any Temporal Cloud resource: resolving the policy, Preflight (decisions that need no Cloud call: Retain, never created).
• controller/namespace/delete.go: the namespace rules (Observed, Decide), matching Temporal Cloud's delete behavior. Only an Active namespace can be deleted; delete protection, Nexus endpoints, and suspension block it; a namespace whose creation time doesn't match the one the operator recorded is never deleted.

Issue #9

[CLAassistant]

All committers have signed the CLA.

[niyomukiza-mechack]

@jaypipes @tomba7 @zainawaisn

1. What should happen on delete when spec.deletionPolicy is not set?
   I'm thinking if the resource was created by the operator, delete and if it was adopted retain.

2. Before sending the delete request, Decide checks for delete protection and Nexus endpoints itself, even though Temporal Cloud already rejects the delete in both cases. Should I keep those checks, or just send the delete and surface Cloud's error? Checking first gives clearer messages and avoids repeated rejected deletes.
   Relying on Cloud means less duplicated logic but its error codes overlap (in both cases temporal returns FailedPrecondition), so we'd have to show its message as is.

[tomba7]

What should happen on delete when spec.deletionPolicy is not set?
I'm thinking if the resource was created by the operator, delete and if it was adopted retain.

IMO we should definitely set a default. My hunch is that most users who are familiar with ACK/Crossplane will expect Delete, since that's the default in those tools. But apart from the familiarity, either option is fine with me. I'd say we go with Delete by default (regardless of how it was created) and then adjust it once we have customer feedback or when we get to the resource adoption implementation.

Before sending the delete request, Decide checks for delete protection and Nexus endpoints itself, even though Temporal Cloud already rejects the delete in both cases. Should I keep those checks, or just send the delete and surface Cloud's error? Checking first gives clearer messages and avoids repeated rejected deletes.
Relying on Cloud means less duplicated logic but its error codes overlap (in both cases temporal returns FailedPrecondition), so we'd have to show its message as is.

On the surface, the delete protection check seems like an obvious one to keep but unsure about the Nexus endpoint one since I haven't reviewed the PR just yet (will do shortly). In general, my preference is to keep the operator's own checks to a minimum, make tcloud the source of truth, and only add operator side checks when it's obvious or necessary (e..g the namespace CRD only has basic validation and leaves the rest to tcloud)

[jaypipes]

getting closer @niyomukiza-mechack! :)

[jaypipes]

See my note above about changing the method signature of types/Resource.ResourceDelete to return a copy of the original resource with any state mutations applied to it. So, here, you'd be doing something like:

	updated := desired.DeepCopy()
	cr = updated.CR().(*tcloudv1alpha1.Namespace)
...

	if err != nil {
		// The gRPC error includes Temporal Cloud's message, for example that
		// deletion protection is enabled.
		condition.SetStalled(
			updated,
			metav1.ConditionTrue,
			err.Error(),
			cloudcommonv1.ReasonResourceDeleteFailed,
		)
...

[jaypipes]

Hmm, interesting, I had not considered this user experience when implementing the annotation.Value(). It's clunky. I think it might be better to change the annotation.Value() implementation to simply return the first matched value and sort the matched keys to ensure deterministic behaviour. That way you can just do:

v, ok := annotation.Value(subject, key)

and won't need to do this alternate code branch to check for multiple values.

[jaypipes]

If you made these an actual type, you could add a Validate() method to the type to eliminate the need for the controller/namespace/delete.go:deletionPolicy() function:

// DeletionPolicy describes the expected behaviour of the controller when
// deleting a Temporal Cloud resource associated with a CR.
type DeletionPolicy string

const (
	// DeletionPolicyDelete deletes the Temporal Cloud resource, and removes
	// the custom resource once Temporal Cloud has finished deleting it.
	DeletionPolicyDelete DeletionPolicy = "delete"
	// DeletionPolicyRetain leaves the Temporal Cloud resource in place and
	// only removes the custom resource.
	DeletionPolicyRetain DeletionPolicy = "retain"
)

// Validate returns an error if the DeletionPolicy is not valid.
func (p DeletionPolicy) Validate() error {
	switch p {
		case DeletionPolicyRetain, DeletionPolicyDelete:
			return nil
		default:
			allowed := strings.Join(
				[]string{
					string(DeletionPolicyRetain),
					string(DeletionPolicyDelete),
				}, "",
			)
			return fmt.Errorf(
				"invalid deletion policy %q. allowed values: %s",
				p,  allowed,
			)
	}
} 

[jaypipes]

See above comment in api/cloud/common/const.go about adding a DeletionPolicy.Validate() method. You could then replace the above with simply:

return v, v.Validate()

[tomba7]

We should use requeue.DefaultRequeueAfterDuration from kube-temporal to be consistent

[tomba7]

Activating and Updating would be obvious ones to requeue here since they are operation in flight that the delete will fail on.

[tomba7]

Would love to see a table test for the deletion policy that validates the delete/retain behavior making/not making calls to tcloud, delete on a namespace with no deletion policy specified picking up the default, etc.