cleanup(reporter): address review follow-ups on unchanged-condition skip by LightCreator1007 · Pull Request #334 · kubernetes-sigs/node-readiness-controller

LightCreator1007 · 2026-08-01T10:58:43Z

Description

Follow-up to #263, addressing the review comments left open when it merged.

Fold the idempotency check into a single early return after the condition search, dropping the needsUpdate flag and the separate transitionTime variable, the split logic was hard to follow.
Replace the block comment with line comments (block comments are a Go anti-pattern), and refer to heartbeatPeriod rather than a hardcoded "5 minutes", since the period is configurable.
Rename test case State change triggers immediate write to update condition on state change — the immediate write is the outcome, not the cause, and is now noted in a comment.
Return a bool instead of an update count from the test helper, collapsing the assertion to one comparison.

No behaviour change.

Related Issue

None

Type of Change

/kind cleanup

Testing

This change is a refactor only.

Checklist

make test passes
make lint passes

Does this PR introduce a user-facing change?

NONE

netlify · 2026-08-01T10:58:48Z

✅ Deploy Preview for node-readiness-controller ready!

Name	Link
🔨 Latest commit	`158bfa3`
🔍 Latest deploy log	https://app.netlify.com/projects/node-readiness-controller/deploys/6a8691cd5e5a7a00084af2b5
😎 Deploy Preview	https://deploy-preview-334--node-readiness-controller.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

kubernetes-prow · 2026-08-01T10:58:52Z

Hi @LightCreator1007. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Tip

We noticed you've done this a few times! Consider joining the org to skip this step and gain /lgtm and other bot rights. We recommend asking approvers on your previous PRs to sponsor you.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

ajaysundark · 2026-08-01T23:04:59Z

Thanks for iterating on this!

/cc @AnuragThePathak as he had the original comments

kubernetes-prow · 2026-08-01T23:05:03Z

@ajaysundark: GitHub didn't allow me to request PR reviews from the following users: had, the, original, AnuragThePathak, as, he.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

Thanks for iterating on this!

/cc @AnuragThePathak as he had the original comments

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

AnuragThePathak

Thanks for following up @LightCreator1007 glad you decided not to wrap up the chapter with merging of the original PR.

ajaysundark · 2026-08-03T06:58:11Z

/cc @AnuragThePathak

ajaysundark · 2026-08-03T06:58:51Z

/ok-to-test

AnuragThePathak · 2026-08-04T19:52:41Z

-			}
-		}
-
-		if !found {


I must admit, this is something I never had my eyes on, and didn't anticipate this change either until I saw this today (the major reason being it was not part of what you changed earlier and existed way before that). This slightly makes me question whether your former approach to use existingCondition was actually better, but considering that involves pointer, and considering simplicity of our current needs I'm approving this.

AnuragThePathak · 2026-08-04T19:54:38Z

/assign @ajaysundark

AnuragThePathak · 2026-08-06T05:51:34Z

/unassign @ajaysundark
/assign
/assign @LightCreator1007

AnuragThePathak

I have two more suggestions, I see that we are not testing two situations now.

Smoke test for node not found error path
Validation of the LastTransitionTime

Since we spent so much time refactoring, I think working on these two and sealing these two will elevate the standard of this method significantly.

AnuragThePathak · 2026-08-06T19:56:27Z


-			countUpdates := func() int {
-				n := 0
+			updateCalled := func() bool {


About this, I gave some thought about this, I think your original approach of counting was better. So please revert this one.

But change the wantUpdateCalled to integer value (update name of that variable too as wantUpdateCalled sounds boolean to anyone).

DsThakurRawat · 2026-08-09T07:23:24Z

Heads up, #367 merged into main about an hour ago and it lands in this same file, so the branch doesn't merge cleanly any more. Checked it against main at d74dabd:

Auto-merging cmd/readiness-condition-reporter/main.go
CONFLICT (content): Merge conflict in cmd/readiness-condition-reporter/main_test.go

main.go is fine. The only conflict is in main_test.go and it's the mechanical kind: both branches appended a new top level test function at the end of the file, so git can't tell which one goes first. main has TestParseDurationWithDefault now, this branch has TestUpdateNodeConditionNodeNotFound.

Keeping both, in either order, clears it. I resolved it that way locally and gofmt, go build ./... and go test ./cmd/readiness-condition-reporter/... all came back clean, so there's nothing behavioural in it.

It might also sort itself out. The review note above suggests folding that case into the existing table rather than keeping a separate test, and if TestUpdateNodeConditionNodeNotFound goes away then so does the conflict.

Sorry about the collision, that was my PR that landed.

AnuragThePathak

Remaining looks good other than the addressed 3

AnuragThePathak · 2026-08-10T16:17:32Z

-				for _, a := range client.Actions() {
-					if a.GetVerb() == "update" && a.GetSubresource() == "status" && a.GetResource().Resource == "nodes" {
-						n++
+			var objs []runtime.Object


var client *fake.Clientset if tt.existingNode != nil { client = fake.NewClientset(tt.existingNode) } else { client = fake.NewClientset() } var previousTransition metav1.Time if tt.existingNode != nil { for _, cond := range tt.existingNode.Status.Conditions { if string(cond.Type) == conditionType { previousTransition = cond.LastTransitionTime break } } } if previousTransition.IsZero() { previousTransition = metav1.NewTime(time.Now()) }

Decouple client and previousTransition.

AnuragThePathak · 2026-08-10T17:02:49Z


 			err := updateNodeCondition(context.Background(), client, nodeName, conditionType, tt.health, tt.heartbeatPeriod)
+			if tt.existingNode == nil {
+				if !apierrors.IsNotFound(err) {


This log/condition won't tell exact failure scenario, as it doesn't differentiate between no error and error being something other than not found.

if err != nil { if tt.wantNotFoundErr { if !apierrors.IsNotFound(err) { t.Fatalf("updateNodeCondition() error = %v, want a NotFound error", err) } if got := countUpdateCalls(client); got != tt.wantUpdateCount { t.Errorf("UpdateStatus called = %v, want %v", got, tt.wantUpdateCount) } return } t.Fatalf("updateNodeCondition() error = %v", err) } if tt.wantNotFoundErr { t.Fatalf("updateNodeCondition() succeeded, want a NotFound error") }

Also introduced wantNotFoundErr because inputs in tests are not often expected to remain the same during the runtime most often.

AnuragThePathak · 2026-08-10T17:04:41Z

+		wantStatus              corev1.ConditionStatus
+		wantReason              string
+		wantUpdateCount         int
+		wantTransitionPreserved bool


@LightCreator1007 update this please

AnuragThePathak

@ajaysundark this one looks good to me. Feel free to merge as per release and other convince as no functional change

ajaysundark · 2026-08-17T04:48:01Z

-				return n
+			}
+			if previousTransition.IsZero() {
+				previousTransition = metav1.NewTime(time.Now())


nit: it'd be good to base previousTransition to a fixed / deterministic timestamp instead of injecting 'now' as you're using it to comparing later.

ajaysundark

@LightCreator1007 Thanks for this nice refactor!

added a nit comment (non blocking) if you could find time to update, please consider handling this.

/hold
/lgtm
/approve

…reserved

ajaysundark

/lgtm

ajaysundark · 2026-08-20T13:15:55Z

Thanks @LightCreator1007 @AnuragThePathak !

AnuragThePathak

LGTM with the update on deterministic previousTransistion. We are not exactly testing the equality of time stamp, time.Now() put by the updateNodeCondition, but that should be fine I believe in current situation I think.

kubernetes-prow · 2026-08-20T19:23:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ajaysundark, AnuragThePathak, LightCreator1007

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [ajaysundark]

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

kubernetes-prow Bot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Aug 1, 2026

kubernetes-prow Bot requested review from dchen1107 and mrunalp August 1, 2026 10:58

kubernetes-prow Bot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 1, 2026

kubernetes-prow Bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 1, 2026

AnuragThePathak reviewed Aug 2, 2026

View reviewed changes

Comment thread cmd/readiness-condition-reporter/main.go Outdated

Comment thread cmd/readiness-condition-reporter/main.go

kubernetes-prow Bot requested a review from AnuragThePathak August 3, 2026 06:58

ajaysundark removed request for AnuragThePathak, dchen1107 and mrunalp August 3, 2026 06:58

kubernetes-prow Bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 3, 2026

AnuragThePathak approved these changes Aug 4, 2026

View reviewed changes

kubernetes-prow Bot assigned ajaysundark Aug 4, 2026

alanhuangch mentioned this pull request Aug 5, 2026

[FEATURE] Add opt-in heartbeat freshness enforcement for continuous Node conditions #357

Closed

AnuragThePathak reviewed Aug 5, 2026

View reviewed changes

Comment thread cmd/readiness-condition-reporter/main.go Outdated

kubernetes-prow Bot assigned AnuragThePathak and LightCreator1007 and unassigned ajaysundark Aug 6, 2026

AnuragThePathak suggested changes Aug 6, 2026

View reviewed changes

AnuragThePathak reviewed Aug 6, 2026

View reviewed changes

kubernetes-prow Bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 9, 2026

AnuragThePathak suggested changes Aug 10, 2026

View reviewed changes

AnuragThePathak approved these changes Aug 14, 2026

View reviewed changes

ajaysundark reviewed Aug 17, 2026

View reviewed changes

ajaysundark approved these changes Aug 17, 2026

View reviewed changes

kubernetes-prow Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 17, 2026

kubernetes-prow Bot assigned ajaysundark Aug 17, 2026

kubernetes-prow Bot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 17, 2026

ajaysundark requested a review from AnuragThePathak August 17, 2026 04:51

kubernetes-prow Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2026

LightCreator1007 added 9 commits August 20, 2026 10:45

cleanup(reporter): address review follow-ups from kubernetes-sigs#263

c1b9440

cleanup(reporter): merged skip check into the search loop

b35b2a5

cleanup(reporter): split condition lookup from the status update

3d2d6f0

cleanup(reporter): removed defensive zero check

0548566

test(reporter): assert write counts, lookup failure and transition time

5abda22

test(reporter): merge the NotFound test into the table and share helpers

ab785d4

test(reporter): declare expected errors in the table and decouple setup

97dcda2

test(reporter): rename wantTransitionPreserved to wantTransitionTimeP…

d18f175

…reserved

test(reporter): use a deterministic baseline for fresh transitions

158bfa3

LightCreator1007 force-pushed the cleanup/reporter-idempotency-gate-followup branch from edc3734 to 158bfa3 Compare August 20, 2026 05:34

kubernetes-prow Bot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 20, 2026

ajaysundark approved these changes Aug 20, 2026

View reviewed changes

kubernetes-prow Bot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 20, 2026

AnuragThePathak approved these changes Aug 20, 2026

View reviewed changes

Uh oh!

Conversation

LightCreator1007 commented Aug 1, 2026

Description

Related Issue

Type of Change

Testing

Checklist

Does this PR introduce a user-facing change?

Uh oh!

netlify Bot commented Aug 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for node-readiness-controller ready!

Uh oh!

kubernetes-prow Bot commented Aug 1, 2026

Uh oh!

ajaysundark commented Aug 1, 2026

Uh oh!

kubernetes-prow Bot commented Aug 1, 2026

Uh oh!

AnuragThePathak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ajaysundark commented Aug 3, 2026

Uh oh!

ajaysundark commented Aug 3, 2026

Uh oh!

AnuragThePathak Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnuragThePathak commented Aug 4, 2026

Uh oh!

Uh oh!

AnuragThePathak commented Aug 6, 2026

Uh oh!

AnuragThePathak left a comment

Choose a reason for hiding this comment

Uh oh!

AnuragThePathak Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

DsThakurRawat commented Aug 9, 2026

Uh oh!

AnuragThePathak left a comment

Choose a reason for hiding this comment

Uh oh!

AnuragThePathak Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

AnuragThePathak Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

AnuragThePathak Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

AnuragThePathak left a comment

Choose a reason for hiding this comment

Uh oh!

ajaysundark Aug 17, 2026

Choose a reason for hiding this comment

Uh oh!

ajaysundark left a comment

Choose a reason for hiding this comment

Uh oh!

ajaysundark left a comment

Choose a reason for hiding this comment

Uh oh!

ajaysundark commented Aug 20, 2026

Uh oh!

AnuragThePathak left a comment

Choose a reason for hiding this comment

Uh oh!

kubernetes-prow Bot commented Aug 20, 2026

Uh oh!

Reviewers

Assignees

Labels

netlify Bot commented Aug 1, 2026 •

edited

Loading

AnuragThePathak Aug 4, 2026 •

edited

Loading