Skip to content

pkg/manifests: fix dropped walk errors - #495

Merged
openshift-merge-bot[bot] merged 2 commits into
operator-framework:masterfrom
alrs:manifest-walk-errs
Jun 26, 2026
Merged

pkg/manifests: fix dropped walk errors#495
openshift-merge-bot[bot] merged 2 commits into
operator-framework:masterfrom
alrs:manifest-walk-errs

Conversation

@alrs

@alrsalrs commented May 29, 2026

Copy link
Copy Markdown
Contributor

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>
CopilotAI review requested due to automatic review settings May 29, 2026 21:41
@openshift-ci
openshift-ciBot requested review from fgiudici and oceanc80May 29, 2026 21:41

CopilotAI left a comment

Copy link
Copy Markdown

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.

FileDescription
pkg/manifests/packagemanifestloader.goPropagates walker errors early in both walk functions before file inspection.
pkg/manifests/bundleloader.goPropagates 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

codecovBot commented Jun 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 22.33%. Comparing base (755d5d1) to head (b087a17).
⚠️ Report is 8 commits behind head on master.

Additional details and impacted files
@@ Coverage Diff @@## master #495 +/- ##
==========================================
+ Coverage 22.13% 22.33% +0.19% 
==========================================
Files 60 60 Lines 7869 7872 +3 ==========================================
+ Hits 1742 1758 +16 + Misses 5969 5962 -7 + Partials 158 152 -6 

☔ View full report in Codecov by Harness.
📢 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.

@tmshorttmshort left a comment

Copy link
Copy Markdown
Contributor

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-ciopenshift-ciBot 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

/hold

@openshift-ciopenshift-ciBot 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

/approve cancel
/hold cancel

I did not mean to LGTM and approve

@openshift-ciopenshift-ciBot removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 2, 2026
@openshift-ciopenshift-ciBot removed the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2026
CopilotAI review requested due to automatic review settings June 16, 2026 11:42
Add unit tests covering walk error propagation in WalkFunc callbacks,
collectWalkErrs integration, and inaccessible subpath scenarios.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Ready to approve

The changes correctly implement standard filepath.WalkFunc error handling and are covered by new unit tests without introducing behavioral risk beyond preserving/propagating existing walk errors.

Note: this review does not count toward required approvals for merging.

Copilot's findings
  • Files reviewed: 3/3 changed files
  • Comments generated: 0 new

Note

Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.

@perdasilva

Copy link
Copy Markdown
Contributor

@tmshort@alrs I've added a commit with unit tests - let me know if this is an improvement or not

@tmshort

Copy link
Copy Markdown
Contributor

/approve

@openshift-ciopenshift-ciBot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 26, 2026

@rashmigottipatirashmigottipati 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.

/lgtm

@openshift-ciopenshift-ciBot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2026
@openshift-ci

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rashmigottipati, tmshort

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-merge-bot
openshift-merge-botBot merged commit 492d6ba into operator-framework:masterJun 26, 2026
8 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@alrs@perdasilva@tmshort@rashmigottipati