fix(declcfg): WriteFS now preserves Others and Deprecations (#1979)

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

Signed-off-by: Todd Short <tshort@redhat.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

Author: tmshort

alpha/declcfg/write.go

@@ -467,52 +467,70 @@ type encoder interface {
 	Encode(interface{}) error
 }
 
-func writeToEncoder(cfg DeclarativeConfig, enc encoder) error {
-	pkgNames := sets.NewString()
+func configsByPackage(cfg DeclarativeConfig) (sets.Set[string], map[string]DeclarativeConfig, []Meta) {
+	pkgNames := sets.New[string]()
+	byCfg := map[string]DeclarativeConfig{}
+	var rootOthers []Meta
+
+	add := func(name string, fn func(DeclarativeConfig) DeclarativeConfig) {
+		if name == "" {
+			return
+		}
+		pkgNames.Insert(name)
+		byCfg[name] = fn(byCfg[name])
+	}
 
-	packagesByName := map[string][]Package{}
 	for _, p := range cfg.Packages {
-		pkgName := p.Name
-		pkgNames.Insert(pkgName)
-		packagesByName[pkgName] = append(packagesByName[pkgName], p)
+		add(p.Name, func(c DeclarativeConfig) DeclarativeConfig {
+			c.Packages = append(c.Packages, p)
+			return c
+		})
 	}
-	channelsByPackage := map[string][]Channel{}
 	for _, c := range cfg.Channels {
-		pkgName := c.Package
-		pkgNames.Insert(pkgName)
-		channelsByPackage[pkgName] = append(channelsByPackage[pkgName], c)
+		add(c.Package, func(dc DeclarativeConfig) DeclarativeConfig {
+			dc.Channels = append(dc.Channels, c)
+			return dc
+		})
 	}
-	bundlesByPackage := map[string][]Bundle{}
 	for _, b := range cfg.Bundles {
-		pkgName := b.Package
-		pkgNames.Insert(pkgName)
-		bundlesByPackage[pkgName] = append(bundlesByPackage[pkgName], b)
+		add(b.Package, func(c DeclarativeConfig) DeclarativeConfig {
+			c.Bundles = append(c.Bundles, b)
+			return c
+		})
 	}
-	othersByPackage := map[string][]Meta{}
 	for _, o := range cfg.Others {
-		pkgName := o.Package
-		pkgNames.Insert(pkgName)
-		othersByPackage[pkgName] = append(othersByPackage[pkgName], o)
+		if o.Package == "" {
+			rootOthers = append(rootOthers, o)
+			continue
+		}
+		add(o.Package, func(c DeclarativeConfig) DeclarativeConfig {
+			c.Others = append(c.Others, o)
+			return c
+		})
 	}
-	deprecationsByPackage := map[string][]Deprecation{}
 	for _, d := range cfg.Deprecations {
-		pkgName := d.Package
-		pkgNames.Insert(pkgName)
-		deprecationsByPackage[pkgName] = append(deprecationsByPackage[pkgName], d)
+		add(d.Package, func(c DeclarativeConfig) DeclarativeConfig {
+			c.Deprecations = append(c.Deprecations, d)
+			return c
+		})
 	}
 
-	for _, pName := range pkgNames.List() {
-		if len(pName) == 0 {
-			continue
-		}
-		pkgs := packagesByName[pName]
-		for _, p := range pkgs {
+	return pkgNames, byCfg, rootOthers
+}
+
+func writeToEncoder(cfg DeclarativeConfig, enc encoder) error {
+	pkgNames, byCfg, rootOthers := configsByPackage(cfg)
+
+	for _, pName := range sets.List(pkgNames) {
+		pkgCfg := byCfg[pName]
+
+		for _, p := range pkgCfg.Packages {
 			if err := enc.Encode(p); err != nil {
 				return err
 			}
 		}
 
-		channels := channelsByPackage[pName]
+		channels := pkgCfg.Channels
 		sort.Slice(channels, func(i, j int) bool {
 			return channels[i].Name < channels[j].Name
 		})
@@ -522,7 +540,7 @@ func writeToEncoder(cfg DeclarativeConfig, enc encoder) error {
 			}
 		}
 
-		bundles := bundlesByPackage[pName]
+		bundles := pkgCfg.Bundles
 		sort.Slice(bundles, func(i, j int) bool {
 			return bundles[i].Name < bundles[j].Name
 		})
@@ -532,7 +550,7 @@ func writeToEncoder(cfg DeclarativeConfig, enc encoder) error {
 			}
 		}
 
-		others := othersByPackage[pName]
+		others := pkgCfg.Others
 		sort.SliceStable(others, func(i, j int) bool {
 			return others[i].Schema < others[j].Schema
 		})
@@ -552,15 +570,14 @@ func writeToEncoder(cfg DeclarativeConfig, enc encoder) error {
 		// Deprecation object for a package, and it would bypass validation if this
 		// function gets called without conversion.
 		//
-		deprecations := deprecationsByPackage[pName]
-		for _, d := range deprecations {
+		for _, d := range pkgCfg.Deprecations {
 			if err := enc.Encode(d); err != nil {
 				return err
 			}
 		}
 	}
 
-	for _, o := range othersByPackage[""] {
+	for _, o := range rootOthers {
 		if err := enc.Encode(o); err != nil {
 			return err
 		}
@@ -572,34 +589,35 @@ func writeToEncoder(cfg DeclarativeConfig, enc encoder) error {
 type WriteFunc func(config DeclarativeConfig, w io.Writer) error
 
 func WriteFS(cfg DeclarativeConfig, rootDir string, writeFunc WriteFunc, fileExt string) error {
-	channelsByPackage := map[string][]Channel{}
-	for _, c := range cfg.Channels {
-		channelsByPackage[c.Package] = append(channelsByPackage[c.Package], c)
-	}
-	bundlesByPackage := map[string][]Bundle{}
-	for _, b := range cfg.Bundles {
-		bundlesByPackage[b.Package] = append(bundlesByPackage[b.Package], b)
-	}
+	pkgNames, byCfg, rootOthers := configsByPackage(cfg)
 
 	if err := os.MkdirAll(rootDir, 0777); err != nil {
 		return err
 	}
 
-	for _, p := range cfg.Packages {
-		fcfg := DeclarativeConfig{
-			Packages: []Package{p},
-			Channels: channelsByPackage[p.Name],
-			Bundles:  bundlesByPackage[p.Name],
+	for _, pName := range sets.List(pkgNames) {
+		if !filepath.IsLocal(pName) {
+			return fmt.Errorf("invalid package name %q: must be a single local path element", pName)
 		}
-		pkgDir := filepath.Join(rootDir, p.Name)
+		pkgDir := filepath.Join(rootDir, pName)
 		if err := os.MkdirAll(pkgDir, 0777); err != nil {
 			return err
 		}
 		filename := filepath.Join(pkgDir, fmt.Sprintf("catalog%s", fileExt))
-		if err := writeFile(fcfg, filename, writeFunc); err != nil {
+		if err := writeFile(byCfg[pName], filename, writeFunc); err != nil {
+			return err
+		}
+	}
+
+	// Others with no package name cannot belong to any package directory;
+	// write them to a root-level catalog file, consistent with writeToEncoder.
+	if len(rootOthers) > 0 {
+		filename := filepath.Join(rootDir, fmt.Sprintf("catalog%s", fileExt))
+		if err := writeFile(DeclarativeConfig{Others: rootOthers}, filename, writeFunc); err != nil {
 			return err
 		}
 	}
+
 	return nil
 }
 

alpha/declcfg/write_test.go

@@ -2,7 +2,10 @@ package declcfg
 
 import (
 	"bytes"
+	"context"
 	"encoding/json"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -621,3 +624,40 @@ class anakin-dark-anakin.v0.1.0 skipped
 		})
 	}
 }
+
+func TestWriteFS(t *testing.T) {
+	cfg := buildValidDeclarativeConfig(validDeclarativeConfigSpec{IncludeUnrecognized: true, IncludeDeprecations: true})
+
+	dir := t.TempDir()
+	require.NoError(t, WriteFS(cfg, dir, WriteJSON, ".json"))
+
+	// Per-package files must include Others and Deprecations for that package.
+	anakinData, err := os.ReadFile(filepath.Join(dir, "anakin", "catalog.json"))
+	require.NoError(t, err)
+	anakinStr := string(anakinData)
+	require.Contains(t, anakinStr, `"schema": "custom.3"`, "anakin catalog must contain Others")
+	require.Contains(t, anakinStr, `"schema": "olm.deprecations"`, "anakin catalog must contain Deprecations")
+
+	bobData, err := os.ReadFile(filepath.Join(dir, "boba-fett", "catalog.json"))
+	require.NoError(t, err)
+	bobStr := string(bobData)
+	require.Contains(t, bobStr, `"schema": "custom.3"`, "boba-fett catalog must contain Others")
+
+	// Others with no package name must appear in a root-level catalog file.
+	rootData, err := os.ReadFile(filepath.Join(dir, "catalog.json"))
+	require.NoError(t, err)
+	rootStr := string(rootData)
+	require.Contains(t, rootStr, `"schema": "custom.1"`, "root catalog must contain package-less Others")
+	require.Contains(t, rootStr, `"schema": "custom.2"`, "root catalog must contain package-less Others")
+
+	// Round-trip: loading the written files must reproduce the Others and Deprecations.
+	// Normalize JSON whitespace in Blob fields before comparing because LoadFS calls
+	// Meta.UnmarshalJSON, which re-encodes the raw blob through a map[string]interface{}
+	// and therefore compacts whitespace and normalizes key order.
+	loaded, err := LoadFS(context.Background(), os.DirFS(dir))
+	require.NoError(t, err)
+	removeJSONWhitespace(&cfg)
+	removeJSONWhitespace(loaded)
+	require.ElementsMatch(t, cfg.Others, loaded.Others)
+	require.ElementsMatch(t, cfg.Deprecations, loaded.Deprecations)
+}