Skip to content

decorators: replace init() panics with lazy error propagation - #3910

Open
sebrandon1 wants to merge 1 commit into
operator-framework:masterfrom
sebrandon1:decorators-replace-panic-with-error
Open

decorators: replace init() panics with lazy error propagation#3910
sebrandon1 wants to merge 1 commit into
operator-framework:masterfrom
sebrandon1:decorators-replace-panic-with-error

Conversation

@sebrandon1

@sebrandon1 sebrandon1 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Replace init() panics with sync.Once lazy initialization for jq query compilation in decorators/operator.go
  • Failures now surface as reconciliation errors in Component.Reference() instead of crashing the operator process at startup
  • Add direct test for jqQueries() verifying successful compilation and sync.Once idempotency

Test Plan

  • go test ./pkg/controller/operators/decorators/... — 9/9 pass
  • go vet clean
  • CI passes

Summary by CodeRabbit

  • Bug Fixes

    • Improved reliability when initializing component and CSV query processing.
    • Query parsing errors are now reported gracefully instead of causing startup failures.
    • Component condition extraction now properly handles initialization errors.
  • Tests

    • Added coverage to verify successful query initialization and consistent reuse of initialized queries.

Convert jq query compilation from panicking in init() to lazy
initialization via sync.Once with proper error returns. Failures
now surface as reconciliation errors in Component.Reference()
rather than crashing the operator process at startup.
@openshift-ci
openshift-ci Bot requested review from dtfranz and pedjak September 1, 2026 13:53
@openshift-ci

openshift-ci Bot commented Sep 1, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign dtfranz for approval. For more information see the Code Review Process.

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

Details Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci Bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 1, 2026
@openshift-ci

openshift-ci Bot commented Sep 1, 2026

Copy link
Copy Markdown

Hi @sebrandon1. Thanks for your PR.

I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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-sigs/prow repository.

@coderabbitai

coderabbitai Bot commented Sep 1, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: c51c7f62-3c12-43b5-b6b7-c17a98be47fa

📥 Commits

Reviewing files that changed from the base of the PR and between 3fce27a and 4aeedbf.

📒 Files selected for processing (2)
  • pkg/controller/operators/decorators/operator.go
  • pkg/controller/operators/decorators/operator_test.go

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The decorator package now initializes component and CSV jq queries lazily and safely with sync.Once. Query parsing errors return from Reference instead of causing package initialization panics. Tests verify successful initialization, non-nil queries, and pointer reuse.

Changes

jq query initialization

Layer / File(s) Summary
Lazy query cache
pkg/controller/operators/decorators/operator.go, pkg/controller/operators/decorators/operator_test.go
jqQueries initializes and caches both jq queries once. Parsing errors are returned. Tests verify successful initialization, non-nil results, and pointer reuse.
Reference query selection
pkg/controller/operators/decorators/operator.go
Reference propagates query initialization errors and selects the CSV-specific query for CSV objects.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 4aeed

The change replaces startup panics with lazy error propagation and adds focused coverage; no actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: pedjak, dtfranz

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: replacing initialization panics with lazy error propagation in the decorators package.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

Some tools did not complete. Review the errors below.

🔧 golangci-lint (2.13.2)

Error: can't load config: can't unmarshal config by viper (flags, file): 1 error(s) decoding:

  • 'output.formats' expected a map, got 'string'
    The command is terminated due to an error: can't load config: can't unmarshal config by viper (flags, file): 1 error(s) decoding:

  • 'output.formats' expected a map, got 'string'


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@rashmigottipati rashmigottipati left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/ok-to-test

@openshift-ci openshift-ci Bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 1, 2026
@tmshort

tmshort commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

I'm concerned that the failure of compiling jq queries is really a coding error, and not a run-time error. There is nothing we can do on a failure to recover. Failing in init() makes sense as it tells the developer immediately that there's a coding error. Continuing despite the error gives a false sense of security to the developer.

In other words, there's no way to recover, so why not fail immediately?

@sebrandon1

Copy link
Copy Markdown
Contributor Author

Good point, generally I try to not use Init() as much as I can just because I don't like logic firing without being explicitly called but I can see what you're getting at. Also it let me add unit tests for that JQ parsing.

I'm good either way if you prefer the original Init() I can just close it. Thanks @tmshort!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Indicates a non-member PR verified by an org member that is safe to test.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants