fix(connection): honor always-update for ready resources

[freeznet]

(If this PR fixes a github issue, please add Fixes #<xyz>.)

Fixes #

(or if this PR is one task of a github issue, please add Master Issue: #<xyz> to link to the master issue.)

Master Issue: #

Motivation

Explain here the context, and why you're making that change. What is the problem you're trying to solve.

Modifications

Describe the modifications you've done.

Verifying this change

• Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

• Added integration tests for end-to-end deployment with large payloads (10MB)
• Extended integration test for recovery after broker failure

Documentation

Check the box below.

Need to update docs?

• doc-required

  (If you need help on updating docs, create a doc issue)

• no-need-doc

  (Please explain why)

• doc

  (If this PR contains doc changes)

[github-actions]

@freeznet:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

[Copilot]

Pull request overview

This PR updates the PulsarConnection reconciliation flow to honor the AlwaysUpdatePulsarResource feature gate so that managed child resources can be re-applied even when they are already Ready=True (useful for upgrade remediation), and documents/exposes that behavior via Helm values and docs.

Changes:

• Continue connection reconciliation for “ready-only” child sets when AlwaysUpdatePulsarResource is enabled and at least one managed child resource is observed.
• Add unit tests covering always-update enabled/disabled behavior and ensuring the deletion guard behavior remains intact.
• Document reconciliation skip semantics and surface the feature toggle guidance in docs and Helm chart metadata.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/connection/reconciler.go	Enables reconciling ready resources when always-update is on; adds helper methods to detect observed children.
pkg/connection/reconciler_test.go	Adds tests validating reconcile behavior with always-update on/off and during connection deletion.
docs/pulsar_resource_lifecycle.md	Documents the reconciliation skip contract and upgrade remediation options (incl. always-update).
docs/pulsar_namespace.md	Adds a note pointing to the lifecycle doc section for reconciliation skip behavior and remediation.
charts/pulsar-resources-operator/values.yaml	Improves Helm values documentation for features.alwaysUpdatePulsarResource.
charts/pulsar-resources-operator/README.md	Updates the chart values table description for features.alwaysUpdatePulsarResource.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[maxsxu]

Reviewed the PR. The reconciler behavior looks sound to me: with AlwaysUpdatePulsarResource enabled it re-enters child reconciliation for observed ready resources, while the deleting-connection remaining-resource guard stays ahead of the always-update path. The focused tests cover the skip path, always-update path, no-op connection/status updates, and deletion guard.

One cleanup item before merge:

• go.sum appears to contain unrelated extra module checksums. I ran go mod tidy -diff on this branch and it wants to remove the added checksum-only churn, including the large cloud.google.com/go/*, Docker, etcd, and other go.mod entries. Since this PR does not change go.mod, please run go mod tidy and commit the resulting go.sum so the dependency metadata stays minimal.

Verification:

• GOCACHE=/private/tmp/pro-go-build-cache GOMODCACHE=/private/tmp/pro-go-mod-cache go test ./pkg/connection passed.
• GOCACHE=/private/tmp/pro-go-build-cache GOMODCACHE=/private/tmp/pro-go-mod-cache go mod tidy -diff reported the go.sum cleanup above.

[maxsxu]

PR Review: fix(connection): honor always-update for ready resources

Summary

This PR makes the PulsarConnectionReconciler honor the AlwaysUpdatePulsarResource feature gate for child resources that are already Ready. Previously, once a resource was Ready, it was never reconciled again — even when AlwaysUpdatePulsarResource was enabled. This is important for upgrade remediation scenarios where a new operator version introduces a new spec field but existing resources remain Ready at the same generation.

The PR also adds several quality-of-life improvements:

• Avoids no-op Kubernetes API writes (both .Update() and .Status().Update()) by comparing before/after state
• Adds comprehensive unit tests covering the new behavior
• Improves documentation for the alwaysUpdatePulsarResource feature and the reconciliation skip contract
• Updates Helm chart descriptions

Positive Aspects

1. Well-structured logic: The shouldReconcileReadyResources() and hasObservedResource() helpers are clean and readable.
2. Good test coverage: Four test scenarios covering disabled, enabled, no-op avoidance, and deletion guard — exactly the right cases.
3. Defensive status updates: The updateConnectionStatusIfChanged pattern using equality.Semantic.DeepEqual is a solid way to avoid unnecessary API calls and potential conflict retries.
4. No-op connection update avoidance: Checking controllerutil.AddFinalizer return value and the connectionChanged flag to skip redundant client.Update() calls is a nice improvement.
5. Documentation: The new pulsar_resource_lifecycle.md section clearly explains the reconciliation skip behavior and recovery options.

Suggestions

1. hasObservedResource completeness: The method checks all 10 resource types, which matches the fields on PulsarConnectionReconciler. This looks complete but is fragile — if a new resource type is added to the struct, someone could forget to add it here. Consider adding a comment noting this coupling, or alternatively, keeping the reconcilers list length as a proxy (though that wouldn't account for empty slices).

2. Deletion path with AlwaysUpdatePulsarResource enabled: When the connection is being deleted (DeletionTimestamp is set) and there are no unready resources but there are remaining resources (tenants, namespaces, etc.), the current code correctly does NOT reconcile — it still just logs the remaining-resource guard. However, the deletion guard only checks tenants, namespaces, topics, and geoReplications. Other resource types like permissions, packages, sinks, sources, functions, and nsIsolationPolicies are not checked. This is pre-existing behavior but worth noting as a potential gap.

3. Test helper setAlwaysUpdatePulsarResource: The helper calls itself recursively in the cleanup — setAlwaysUpdatePulsarResource → t.Cleanup(func() { setAlwaysUpdatePulsarResource(t, false) }) — but the inner call is to the unwrapped setAlwaysUpdatePulsarResource (without ForTest), so this is correct but slightly confusing naming. Minor style nit.

4. go.sum churn: The go.sum diff adds 243 lines with zero deletions. This is a large expansion of indirect dependency checksums. It's likely caused by a Go toolchain or dependency update. Not a blocker, but it's worth confirming this is expected and not an accidental go mod tidy with a different Go version.

5. PR description: The PR body is the default template with no actual content filled in (motivation, modifications, verification are all placeholders). It would be helpful to fill this in for future reference.

Verdict

The core logic is sound, the tests are well-designed, and the documentation additions are valuable. The no-op avoidance is a nice touch that reduces unnecessary API server load. LGTM overall — the suggestions above are minor and non-blocking.