fix(declcfg): WriteFS now preserves Others and Deprecations by tmshort · Pull Request #1979 · operator-framework/operator-registry

tmshort · 2026-05-11T20:53:59Z

WriteFS was iterating over Packages, Channels, and Bundles, but never consulted cfg.Others or cfg.Deprecations when building the per-package DeclarativeConfig written to disk. Any custom FBC blobs (e.g. olm.lifecycle metadata used by the Operator Update Planner) and olm.deprecations entries were silently dropped.

This matches the existing behaviour of writeToEncoder, which already handles all five schema types correctly. Others with an empty Package field are now written to a root-level catalog file, consistent with how writeToEncoder emits them at the end of its output.

Fixes: OCPBUGS-85347

Description of the change:

Motivation for the change:

Reviewer Checklist

Implementation matches the proposed design, or proposal is updated to match implementation
Sufficient unit test coverage
Sufficient end-to-end test coverage
Docs updated or added to /docs
Commit messages sensible and descriptive

Copilot

Pull request overview

This PR fixes WriteFS in alpha/declcfg so it no longer drops cfg.Others (custom/unrecognized FBC objects) and cfg.Deprecations when writing per-package declarative config files to disk, aligning its behavior with the existing writeToEncoder path.

Changes:

Include Others and Deprecations in the per-package DeclarativeConfig written by WriteFS.
Write package-less Others to a root-level catalog.<ext> file (consistent with writeToEncoder behavior).
Add a WriteFS unit test validating on-disk contents and a LoadFS round-trip for Others/Deprecations.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
`alpha/declcfg/write.go`	Ensures `WriteFS` carries through `Others`/`Deprecations` and emits package-less `Others` at the catalog root.
`alpha/declcfg/write_test.go`	Adds coverage to prevent regressions and validates `WriteFS` output + `LoadFS` round-trip.

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

codecov · 2026-05-11T20:59:54Z

Codecov Report

❌ Patch coverage is 86.79245% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.88%. Comparing base (3855305) to head (e132825).
⚠️ Report is 13 commits behind head on master.

Files with missing lines	Patch %	Lines
alpha/declcfg/write.go	86.79%	3 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1979      +/-   ##
==========================================
+ Coverage   57.71%   57.88%   +0.16%     
==========================================
  Files         139      140       +1     
  Lines       13373    13448      +75     
==========================================
+ Hits         7718     7784      +66     
- Misses       4468     4473       +5     
- Partials     1187     1191       +4

☔ 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.

grokspawn · 2026-05-11T21:00:25Z

Can we converge implementation with writeToEncoder?
Existing WriteJSON & WriteYAML methods there use a unified output approach which includes all currently-known schemas, plus a single touchpoint to make future schemas available easily.

Copilot

Pull request overview

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

Comments suppressed due to low confidence (1)

alpha/declcfg/write.go:623

WriteFS now derives pName from all schema types (channels/bundles/others/deprecations) and uses it directly in filepath.Join(rootDir, pName). Because LoadFS does not validate Channel.Package/Bundle.Package/etc, a malicious declarative config could set package to values like "../../outside" or an absolute path and cause WriteFS to write outside rootDir (directory traversal). Please validate/sanitize pName before creating directories (e.g., enforce DNS1123 label like ConvertToModel does, or reject any name that is empty/"."/".." or contains path separators).

	for _, pName := range pkgNames.List() {
		if pName == "" {
			continue
		}
		fcfg := DeclarativeConfig{
			Packages:     packagesByName[pName],
			Channels:     channelsByPackage[pName],
			Bundles:      bundlesByPackage[pName],
			Others:       othersByPackage[pName],
			Deprecations: deprecationsByPackage[pName],
		}
		pkgDir := filepath.Join(rootDir, pName)
		if err := os.MkdirAll(pkgDir, 0777); err != nil {
			return err
		}
		filename := filepath.Join(pkgDir, fmt.Sprintf("catalog%s", fileExt))

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

grokspawn · 2026-05-12T15:26:32Z

-func writeToEncoder(cfg DeclarativeConfig, enc encoder) error {
-	pkgNames := sets.NewString()
+// configsByPackage groups a DeclarativeConfig into per-package sub-configs plus a
+// package-less remainder (Others with no Package field).  It is the single touchpoint


Please remodel/remove the comment here. I'm not comfortable with a claude-based rewording of my review comment.

I did have to clarify what you wanted, and I hope I was correct.

I think you absolutely were. Appreciate it.

WriteFS was iterating over Packages, Channels, and Bundles, but never consulted cfg.Others or cfg.Deprecations when building the per-package DeclarativeConfig written to disk. Any custom FBC blobs (e.g. olm.lifecycle metadata used by the Operator Update Planner) and olm.deprecations entries were silently dropped. The implementation is converged with writeToEncoder by extracting a shared configsByPackage helper that groups all five schema types by package name. Both writeToEncoder and WriteFS now call this single function, so adding a new schema type to DeclarativeConfig in the future requires only one edit. Others with an empty Package field are returned as a package-less remainder and written to a root-level catalog file, matching writeToEncoder's behaviour. Fixes: OCPBUGS-85347 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Todd Short <tshort@redhat.com>

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

grokspawn · 2026-05-12T16:41:22Z


-func writeToEncoder(cfg DeclarativeConfig, enc encoder) error {
-	pkgNames := sets.NewString()
+func configsByPackage(cfg DeclarativeConfig) (sets.Set[string], map[string]DeclarativeConfig, []Meta) {


nit: This triple-return really feels like a wanna-be struct.

grokspawn · 2026-05-12T16:42:53Z

/approve

openshift-ci · 2026-05-12T16:42:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grokspawn

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:

~~OWNERS~~ [grokspawn]

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

joelanford · 2026-05-12T19:08:37Z

/lgtm

Copilot AI review requested due to automatic review settings May 11, 2026 20:54

openshift-ci Bot requested review from camilamacedo86 and pedjak May 11, 2026 20:54

Copilot started reviewing on behalf of tmshort May 11, 2026 20:54 View session

Copilot AI reviewed May 11, 2026

View reviewed changes

Comment thread alpha/declcfg/write_test.go Outdated

tmshort force-pushed the fix-OCPBUGS-85347 branch 2 times, most recently from 5e24fa4 to cc6a25b Compare May 11, 2026 21:06

Copilot AI review requested due to automatic review settings May 11, 2026 21:06

Copilot started reviewing on behalf of tmshort May 11, 2026 21:07 View session

Copilot AI reviewed May 11, 2026

View reviewed changes

tmshort force-pushed the fix-OCPBUGS-85347 branch from cc6a25b to d0d7a08 Compare May 11, 2026 21:11

joelanford reviewed May 11, 2026

View reviewed changes

Comment thread alpha/declcfg/write.go Outdated

Copilot AI review requested due to automatic review settings May 12, 2026 13:26

tmshort force-pushed the fix-OCPBUGS-85347 branch from d0d7a08 to 823717c Compare May 12, 2026 13:26

Copilot started reviewing on behalf of tmshort May 12, 2026 13:27 View session

Copilot AI reviewed May 12, 2026

View reviewed changes

Comment thread alpha/declcfg/write.go Outdated

tmshort force-pushed the fix-OCPBUGS-85347 branch from 823717c to ef0ced1 Compare May 12, 2026 13:59

grokspawn reviewed May 12, 2026

View reviewed changes

tmshort force-pushed the fix-OCPBUGS-85347 branch from ef0ced1 to e132825 Compare May 12, 2026 15:52

Copilot AI review requested due to automatic review settings May 12, 2026 15:52

Copilot started reviewing on behalf of tmshort May 12, 2026 16:02 View session

Copilot AI reviewed May 12, 2026

View reviewed changes

grokspawn reviewed May 12, 2026

View reviewed changes

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 12, 2026

openshift-ci Bot assigned joelanford May 12, 2026

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label May 12, 2026

openshift-merge-bot Bot merged commit e7d7929 into operator-framework:master May 12, 2026
17 checks passed

tmshort deleted the fix-OCPBUGS-85347 branch May 12, 2026 19:14

Conversation

tmshort commented May 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

codecov Bot commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

grokspawn commented May 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

grokspawn May 12, 2026

Choose a reason for hiding this comment

Uh oh!

tmshort May 12, 2026

Choose a reason for hiding this comment

Uh oh!

grokspawn May 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

grokspawn May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grokspawn commented May 12, 2026

Uh oh!

openshift-ci Bot commented May 12, 2026

Uh oh!

joelanford commented May 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented May 11, 2026 •

edited

Loading

grokspawn May 12, 2026 •

edited

Loading