Skip to content

pkg/manifests: fix dropped walk errors#495

Open
alrs wants to merge 1 commit into
operator-framework:masterfrom
alrs:manifest-walk-errs
Open

pkg/manifests: fix dropped walk errors#495
alrs wants to merge 1 commit into
operator-framework:masterfrom
alrs:manifest-walk-errs

Conversation

@alrs
Copy link
Copy Markdown
Contributor

@alrs alrs commented May 29, 2026

A filepath.Walk() function takes an error as one of its arguments. Usually the first thing to do in a Walk() is to check if the err provided is not nil and should be returned. This fixes three places in pkg/manifests where a WalkFunc() was not handling the err.

Tests continue to pass.

Signed-off-by: Lars Lehtonen <lars.lehtonen@gmail.com>
Copilot AI review requested due to automatic review settings May 29, 2026 21:41
@openshift-ci openshift-ci Bot requested review from fgiudici and oceanc80 May 29, 2026 21:41
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds early error handling at the start of filepath.WalkFunc implementations to propagate errors received from the walker before attempting further processing.

Changes:

  • Return early when the incoming err parameter is non-nil in LoadPackagesWalkFunc and LoadBundleWalkFunc in packagemanifestloader.go.
  • Apply the same early-return guard in bundleLoader.LoadBundleWalkFunc.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
pkg/manifests/packagemanifestloader.go Propagates walker errors early in both walk functions before file inspection.
pkg/manifests/bundleloader.go Propagates walker errors early in the bundle loader's walk function.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@perdasilva
Copy link
Copy Markdown
Contributor

@alrs Thanks for this ^^ the functions changed are exported - should we add a unit test for each for the changes?

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 1, 2026

Codecov Report

❌ Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 22.12%. Comparing base (755d5d1) to head (70820bf).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
pkg/manifests/packagemanifestloader.go 0.00% 2 Missing and 2 partials ⚠️
pkg/manifests/bundleloader.go 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #495      +/-   ##
==========================================
- Coverage   22.13%   22.12%   -0.02%     
==========================================
  Files          60       60              
  Lines        7869     7875       +6     
==========================================
  Hits         1742     1742              
- Misses       5969     5972       +3     
- Partials      158      161       +3     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

@tmshort tmshort left a comment

Choose a reason for hiding this comment

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

/lgtm

Looks fine to me, these types of walk functions should be returning the error they receive as input.

@openshift-ci openshift-ci Bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 2, 2026
@tmshort
Copy link
Copy Markdown
Contributor

tmshort commented Jun 2, 2026

/hold

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 2, 2026
@tmshort
Copy link
Copy Markdown
Contributor

tmshort commented Jun 2, 2026

/approve cancel
/hold cancel

I did not mean to LGTM and approve

@openshift-ci openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 2, 2026
@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Jun 2, 2026

[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 ask for approval from tmshort. 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 removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 2, 2026
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.

4 participants