❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

• 🚀 /bns:deploy to deploy the environment

Codecov Report

❌ Patch coverage is 91.66667% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.26%. Comparing base (03ac864) to head (7c73bd8).
⚠️ Report is 37 commits behind head on master.

@@            Coverage Diff             @@
##           master   #24021      +/-   ##
==========================================
+ Coverage   60.25%   60.26%   +0.01%     
==========================================
  Files         347      347              
  Lines       59354    59366      +12     
==========================================
+ Hits        35761    35775      +14     
+ Misses      20724    20719       -5     
- Partials     2869     2872       +3     

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

This generally LGTM, but can we add a test for this?

@blakepettersson I’m not well-versed in Go, especially testing patterns. From what I’ve read, if I wanted to know whether GetSettings was being called during certain flows, I’d need to mock SettingsManager for the tests. But to do that, SettingsManager would need to be an interface. Making that change seems … non-trivial.

Is there a simpler way to count method calls? Or am I off-base on your request?

@blakepettersson 892de07 adds an ArgoDBSettingsSource interface that gets mocked by new tests that check whether or not GetSettings is called under the various conditions.

I’m totally open to alternatives if there’s a more project-standard way of doing this.

@mtbennett-godaddy I wonder if an approach similar to #22282 wouldn't be preferable, since while your approach works, it means that any other future invocation that doesn't follow your pattern would surface this issue again. If we fix this at its core rather than trying to prevent excessive GetSettings calls we would ensure that this would not be an issue in the future.

@blakepettersson Honestly, my top priority here was to reduce the Loading TLS configuration from secret logging. (We get literally hundreds of those messages every time the app set controller reconciles. It’s just overwhelming noise in the logs.) #22282 doesn’t lessen the frequency of that message. It actually increases it. It would need to be updated to remove the log call in updateSettingsFromSecret in lieu of the one in the cache miss branch.

#22282 seems abandoned? Want me to close this PR and open a new one using #22282’s approach with tests?

Superseded by #24080