Skip to content

Replace panic with error return in Policy()#52

Closed
sebrandon1 wants to merge 1 commit into
openshift:mainfrom
sebrandon1:replace-policy-panic-with-error
Closed

Replace panic with error return in Policy()#52
sebrandon1 wants to merge 1 commit into
openshift:mainfrom
sebrandon1:replace-policy-panic-with-error

Conversation

@sebrandon1
Copy link
Copy Markdown
Member

@sebrandon1 sebrandon1 commented Apr 27, 2026

Summary

  • Change Policy() from *ComponentPolicy to (*ComponentPolicy, error) so callers can handle parse failures gracefully instead of crashing
  • Update main.go to log the error and exit with code 1
  • Add testPolicy(t) test helper that calls t.Fatal on error

Test plan

  • go build ./... passes
  • go test ./... passes
  • go vet ./... passes

@openshift-ci openshift-ci Bot requested review from rhmdnd and smith-xyz April 27, 2026 16:01
@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2026
@sebrandon1 sebrandon1 force-pushed the replace-policy-panic-with-error branch from 7737b0e to 7552ff5 Compare May 5, 2026 17:41
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 5, 2026
@sebrandon1
Replace panic with error return in Policy()
0a27370 
Policy() previously called panic() on YAML parse failures or invalid
regex patterns in policy rules. Return an error instead so callers can
handle it gracefully rather than crashing the process.
@sebrandon1 sebrandon1 force-pushed the replace-policy-panic-with-error branch from 7552ff5 to 0a27370 Compare May 7, 2026 16:38
@smith-xyz
Copy link
Copy Markdown
Contributor

smith-xyz commented May 7, 2026

/approve

@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented May 7, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sebrandon1, smith-xyz

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

The pull request process is described 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 approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 7, 2026
@smith-xyz
Copy link
Copy Markdown
Contributor

smith-xyz commented May 7, 2026

/lgtm

note on the jobs: we're adjusting ci to only run a smoke tests for PRs for faster iteration

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label May 7, 2026
@sebrandon1
Copy link
Copy Markdown
Member Author

sebrandon1 commented May 7, 2026

Sounds good! Thanks @smith-xyz

@smith-xyz
Copy link
Copy Markdown
Contributor

smith-xyz commented May 7, 2026

/test images

@openshift-merge-bot
Copy link
Copy Markdown
Contributor

openshift-merge-bot Bot commented May 7, 2026

/retest-required

Remaining retests: 0 against base HEAD eee0a3a and 2 for PR HEAD 0a27370 in total

@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented May 7, 2026

@sebrandon1: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/images 0a27370 link true /test images
ci/prow/default-tls 0a27370 link false /test default-tls
ci/prow/tls13-adherence 0a27370 link false /test tls13-adherence

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@sebrandon1 sebrandon1 closed this May 11, 2026
@sebrandon1
Copy link
Copy Markdown
Member Author

sebrandon1 commented May 11, 2026

Closing in favor of #60 which now includes this change (Policy() returns error instead of panicking) alongside the slog logging migration.

@sebrandon1 sebrandon1 mentioned this pull request May 11, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rhmdnd rhmdnd Awaiting requested review from rhmdnd

@smith-xyz smith-xyz Awaiting requested review from smith-xyz

Assignees

@smith-xyz smith-xyz

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sebrandon1 @smith-xyz