Skip to content

Conversation

@joelanford
Copy link
Member

Fixes #1271

Description

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

@joelanford joelanford requested a review from a team as a code owner September 16, 2024 21:05
@netlify
Copy link

netlify bot commented Sep 16, 2024

Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit 01a7166
🔍 Latest deploy log https://app.netlify.com/sites/olmv1/deploys/66e8a8567b9b6a000892b294
😎 Deploy Preview https://deploy-preview-1276--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@codecov
Copy link

codecov bot commented Sep 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.49%. Comparing base (f0c761d) to head (01a7166).
Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1276      +/-   ##
==========================================
- Coverage   76.51%   76.49%   -0.02%     
==========================================
  Files          40       39       -1     
  Lines        2363     2361       -2     
==========================================
- Hits         1808     1806       -2     
  Misses        389      389              
  Partials      166      166              
Flag Coverage Δ
e2e 57.89% <0.00%> (+0.04%) ⬆️
unit 53.02% <100.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@joelanford joelanford force-pushed the use-cr-terminal-error branch from a66b6c3 to 01a7166 Compare September 16, 2024 21:51
Copy link
Member

@LalatenduMohanty LalatenduMohanty left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 17, 2024
@LalatenduMohanty
Copy link
Member

This makes sense, was not aware of func TerminalError

@everettraven
Copy link
Contributor

I don't think it needs to block this PR since it could be done in a follow-up, but it seems like we never made use of the unrecoverable error in the ClusterExtensionReconciler and we should probably update that.

@joelanford
Copy link
Member Author

@everettraven the beauty of this error type is that controller-runtime type checks for it after we return from Reconcile and does the same logic that we did (but with a few extras like incrementing a different metric)

https://github.com/kubernetes-sigs/controller-runtime/blob/2eb879f25c4829825e6f2511cda035de41ec7030/pkg/internal/controller/controller.go#L306-L310

@joelanford joelanford added this pull request to the merge queue Sep 17, 2024
Merged via the queue into operator-framework:main with commit d144146 Sep 17, 2024
17 of 18 checks passed
@skattoju skattoju mentioned this pull request Sep 25, 2024
4 tasks
@joelanford joelanford deleted the use-cr-terminal-error branch October 22, 2024 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Instead of custom Unrecoverable error, use controller-runtime's TerminalError

3 participants