✨ Add NewWorkApplierWithRuntimeClient constructor

[mkolesnik]

The README documents a NewWorkApplierWithRuntimeClient implementation using sigs.k8s.io/controller-runtime/pkg/client, but users cannot implement it externally because all WorkApplier fields are unexported.
Promote it to an exported constructor.

Closes #226

Summary by CodeRabbit

• New Features

  • Added runtime-client support for work applier to manage manifest work creation, updates, deletion, and retrieval with improved state synchronization.

• Tests

  • Expanded tests for work lifecycle: creation, patching, deletion, cache-hit scenarios, drift correction, and error handling.

• Documentation

  • Updated README to document work builder and applier usage, including both constructor options.

[coderabbitai]

Walkthrough

Adds NewWorkApplierWithRuntimeClient to back WorkApplier with a controller-runtime client; implements get/create/patch/delete via runtime client calls and includes tests covering create vs patch, annotation reconciliation, cache-hit/no-write behavior, generation-based drift correction, and delete semantics.

Changes

Runtime Client Constructor and Tests

Layer / File(s)	Summary
Runtime client constructor and operation wiring pkg/apis/work/v1/applier/workapplier.go	Added NewWorkApplierWithRuntimeClient(workClient client.Client) implementing getWork, createWork, patchWork (RawPatch + post-patch Get), and deleteWork using controller-runtime client.Client.
Test helpers and runtime client test suite pkg/apis/work/v1/applier/workapplier_test.go, pkg/apis/work/v1/README.md	Updated tests: added imports and helpers (getWork, assertWorkState), implemented TestWorkApplierWithRuntimeClient (create, patch on spec/annotation changes, cache-hit behavior, generation drift correction, delete/idempotency), trimmed trailing newlines in newFakeWork, and updated README constructors text.

---

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description clearly states the problem, references the linked issue, and explains the reason for the change, though it lacks formal section headers from the template.
Linked Issues check	✅ Passed	The PR fully addresses issue #226 by exporting NewWorkApplierWithRuntimeClient constructor with proper implementation using controller-runtime client.Client, eliminating the dual-cache race condition.
Out of Scope Changes check	✅ Passed	All changes are directly related to implementing the NewWorkApplierWithRuntimeClient constructor and updating documentation; no unrelated modifications detected.
Title check	✅ Passed	The title clearly and specifically identifies the main change: adding a new exported constructor function NewWorkApplierWithRuntimeClient to the WorkApplier API.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

pkg/apis/work/v1/applier/workapplier.go (1)

75-82: ⚡ Quick win

Post-patch Get is likely redundant and weakens cache update semantics.

controller-runtime's Patch decodes the server response back into the passed object, so work should already hold the updated ManifestWork after the Patch call succeeds. The extra Get adds an API round-trip on the apply hot path, and—more importantly—introduces a failure mode: if this Get errors transiently after the Patch already succeeded, patchWork returns an error, so Apply skips w.cache.updateCache(...) (lines 151-154) even though the write landed. That defeats the cache and forces a redundant re-patch on the next reconcile.

♻️ Drop the redundant Get

 		patchWork: func(ctx context.Context, namespace, name string, pt types.PatchType, data []byte) (*workapiv1.ManifestWork, error) {
 			work := &workapiv1.ManifestWork{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      name,
 					Namespace: namespace,
 				},
 			}
 			if err := workClient.Patch(ctx, work, client.RawPatch(pt, data)); err != nil {
 				return nil, err
 			}
-			if err := workClient.Get(ctx, types.NamespacedName{Namespace: namespace, Name: name}, work); err != nil {
-				return nil, err
-			}
 			return work, nil
 		},

Does sigs.k8s.io/controller-runtime client.Patch (v0.23.1) decode the server response back into the passed object, so the object holds the updated state after Patch returns?

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/apis/work/v1/applier/workapplier.go` around lines 75 - 82, The post-patch
fetch in patchWork is redundant because workClient.Patch already updates the
passed ManifestWork object with the server response. Remove the follow-up
workClient.Get call in patchWork and return the patched work directly after a
successful Patch. Keep the existing error handling for Patch itself, and ensure
Apply still reaches w.cache.updateCache(...) using the already-updated work
object.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@pkg/apis/work/v1/applier/workapplier.go`:
- Around line 75-82: The post-patch fetch in patchWork is redundant because
workClient.Patch already updates the passed ManifestWork object with the server
response. Remove the follow-up workClient.Get call in patchWork and return the
patched work directly after a successful Patch. Keep the existing error handling
for Patch itself, and ensure Apply still reaches w.cache.updateCache(...) using
the already-updated work object.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3f0d3df9-3bea-4062-b116-d39c6c7930e6

📥 Commits

Reviewing files that changed from the base of the PR and between 01db48a and a69abfb.

📒 Files selected for processing (3)

• pkg/apis/work/v1/README.md
• pkg/apis/work/v1/applier/workapplier.go
• pkg/apis/work/v1/applier/workapplier_test.go

✅ Files skipped from review due to trivial changes (1)

• pkg/apis/work/v1/README.md

🚧 Files skipped from review as they are similar to previous changes (1)

• pkg/apis/work/v1/applier/workapplier_test.go

[mikeshng]

/lgtm

This change should be quite safe.

/assign @tesshuflower

PTAL as well. Thanks!

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mikeshng, mkolesnik
Once this PR has been reviewed and has the lgtm label, please assign deads2k for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment