Fix resource state handling and error propagation in reconciler

[michaelhtm]

Issue #2682

Changes Made

reconciler.go:

• Improved error handling: Initialize latest with desired.DeepCopy() to ensure consistent resource state is returned on errors
• Fixed return values: Use named return parameters and return appropriate resource state in all error paths
• Enhanced late initialization: Separate late initialization result handling to prevent state loss
• Cleaner variable management: Removed redundant variable declarations

util.go:

• Fixed adoption fields extraction: Return proper error when annotation is missing instead of empty string
• Simplified field lookup: Use direct map access instead of iteration for better performance
• Corrected JSON unmarshaling: Fix pointer usage in unmarshaling operation

reconciler_test.go:

• Updated test mocks: Align mock expectations with new resource state handling
• Fixed condition assertions: Correct expected condition states for error scenarios
• Enhanced error path testing: Better validation of resource states during error conditions

Impact

• Ensures consistent resource state is always returned from reconciler operations
• Improves error handling reliability and debugging capabilities
• Fixes potential nil pointer issues in error scenarios
• Better test coverage alignment with actual implementation behavior

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[a-hilaly]

/retest

[a-hilaly]

Thanks michael. Can we have an e2e tests for this either for ecr, s3 or both?

/lgtm

[knottnt]

/retest

[michaelhtm]

Tests will not pass for some controllers since they were not merged with the latest code-generator version, where we match the interface changes

[knottnt]

Q: What's the impact of return a copy of desired vs nil here?

[michaelhtm]

latest is a deepCopy of desired.

[knottnt]

I've got that part. I'm wondering what the difference in behavior that returning latest instead of nil is expected to have. Is to ensure that HandleReconcilerErrors doesn't skip patching the resource due to the ackcompare.IsNotNil check?

[michaelhtm]

that's correct

[knottnt]

Thanks @michaelhtm

[knottnt]

/lgtm

[ack-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-hilaly, knottnt, michaelhtm

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [a-hilaly,knottnt,michaelhtm]

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

[ack-prow]

@michaelhtm: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
verify-attribution	13caeb5	link	false	/test verify-attribution
sagemaker-controller-test	13caeb5	link	true	/test sagemaker-controller-test
sqs-controller-test	13caeb5	link	true	/test sqs-controller-test
ec2-controller-test	13caeb5	link	true	/test ec2-controller-test

Full PR test history. Your PR dashboard.

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/test-infra repository. I understand the commands that are listed here.