operator-framework / helm-operator-plugins

Experimental refactoring of the operator-framework's helm operator
Apache License 2.0
49 stars 49 forks source link

fix: error being silenced during apply #395

Closed timonwong closed 1 week ago

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

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

codecov-commenter commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

:exclamation: 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 ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

komish commented 1 week ago

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

timonwong commented 1 week ago

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