Skip to content

fix: error being silenced during apply #395

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Oct 10, 2024
Merged

fix: error being silenced during apply #395

merged 5 commits into from
Oct 10, 2024

Conversation

timonwong
Copy link
Contributor

@timonwong timonwong commented Sep 19, 2024 •
edited
Loading 

	defer func() {
		applyErr := u.Apply(ctx, obj)
   		// Problem: err is not assigned to return value  
		if err == nil && !apierrors.IsNotFound(applyErr) {
			err = applyErr
		}
	}()

porridge
porridge approved these changes Sep 19, 2024
View reviewed changes
Copy link
Member

@porridge porridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!
Although it feels like such issue should really be caught by unit tests. Could you maybe take a stab at adding such case?

@timonwong
Copy link
Contributor Author

Nice catch! Although it feels like such issue should really be caught by unit tests. Could you maybe take a stab at adding such case?

sure! i'll then add a case for that

@timonwong
fix: error being silenced during apply
1c63d8d 
Signed-off-by: Tianpeng Wang <tpwang@alauda.io>
@timonwong
add test
8ad278b 
Signed-off-by: Tianpeng Wang <tpwang@alauda.io>
porridge
porridge suggested changes Sep 19, 2024
View reviewed changes
Copy link
Member

@porridge porridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but the comments seem a bit off.

timonwong and others added 3 commits September 19, 2024 16:12
@timonwong @porridge
Apply suggestions from code review
d4f8eee 
Co-authored-by: Marcin Owsiany <marcin@owsiany.pl>
@timonwong
fix lint
7e5adfc 
Signed-off-by: Tianpeng Wang <tpwang@alauda.io>
@timonwong
check error status message
abbabcb 
Signed-off-by: Tianpeng Wang <tpwang@alauda.io>
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.67%. Comparing base (08ab7fb) to head (abbabcb).
Report is 48 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (08ab7fb) and HEAD (abbabcb). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (08ab7fb) HEAD (abbabcb)
2 1
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #395      +/-   ##
==========================================
- Coverage   85.06%   79.67%   -5.40%     
==========================================
  Files          19       31      +12     
  Lines        1346     1958     +612     
==========================================
+ Hits         1145     1560     +415     
- Misses        125      310     +185     
- Partials       76       88      +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@timonwong timonwong requested a review from porridge October 8, 2024 04:53
@komish
Copy link

komish commented Oct 8, 2024

Any particular reason why the Apply operation can't be done at the end of the function instead of in a defer clause?

@timonwong
Copy link
Contributor Author

timonwong commented Oct 9, 2024 •
edited
Loading

Any particular reason why the Apply operation can't be done at the end of the function instead of in a defer clause?

Becasue the Apply() is for status update here. When early return (in case of error) from Reconcile loop, you need to set error conditions for your managed CR resources, the defer approch is more ideal than:

  1. use goto to handle them in a uniform way
  2. call Apply every time before return err

perdasilva
perdasilva approved these changes Oct 10, 2024
View reviewed changes
Copy link
Contributor

@perdasilva perdasilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! Thank you <3

@perdasilva perdasilva added this pull request to the merge queue Oct 10, 2024
Merged via the queue into operator-framework:main with commit 4b2b362 Oct 10, 2024
6 checks passed
@timonwong timonwong deleted the fix-silent-error branch October 11, 2024 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@perdasilva perdasilva perdasilva approved these changes

@joelanford joelanford Awaiting requested review from joelanford

+1 more reviewer

@porridge porridge porridge approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
area/sdk area/testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@timonwong @codecov-commenter @komish @porridge @perdasilva