feat(PL-6287): extract config to catalog#284
Conversation
327c537 to
28ef879
Compare
28ef879 to
2f71b0a
Compare
📝 WalkthroughWalkthroughAdds a Kubernetes-style Catalog API resource, centralizes API schema identifiers, extracts shared Helm chart types, and loads and merges ChangesCatalog configuration flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Load
participant CatalogResource
participant JoyConfig
participant CatalogConfig
Load->>CatalogResource: load catalog.yaml
Load->>JoyConfig: load joy.yaml
Load->>CatalogConfig: merge chart refs and default chart
CatalogConfig-->>Load: validate merged configuration
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/config/config.go (1)
195-228: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider validating catalog.yaml's
kindagainst the new schema identity.
Loadnever checkscatalogResource.Kind(orAPIVersion, once decodable — see catalog.go comment) againstv1alpha1.CatalogGK/CatalogGVKbefore merging. A catalog.yaml with a wrongkind(e.g. a typo, or accidentally pointing at another resource type) would currently be merged silently as long as its shape happens to matchCatalogSpec.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/config.go` around lines 195 - 228, `Load` currently merges catalog.yaml without verifying that `catalogResource` is actually a Catalog object. Add a kind/apiVersion identity check in the `Load` flow before calling `MergeCatalogResourceIntoCatalogConfig`, using `catalogResource.Kind` (and `APIVersion` once available) against `v1alpha1.CatalogGK`/`CatalogGVK`, and return a clear validation error on mismatch so malformed or wrong resource types are not merged silently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/config/config.go`:
- Around line 195-228: `Load` currently merges catalog.yaml without verifying
that `catalogResource` is actually a Catalog object. Add a kind/apiVersion
identity check in the `Load` flow before calling
`MergeCatalogResourceIntoCatalogConfig`, using `catalogResource.Kind` (and
`APIVersion` once available) against `v1alpha1.CatalogGK`/`CatalogGVK`, and
return a clear validation error on mismatch so malformed or wrong resource types
are not merged silently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 49d14033-f444-4a1a-9263-8679cbc214d8
📒 Files selected for processing (9)
api/v1alpha1/catalog.goapi/v1alpha1/environment.goapi/v1alpha1/project.goapi/v1alpha1/release.goapi/v1alpha1/scheme.gointernal/config/config.gointernal/config/config_test.gointernal/helm/helm.gopkg/helm/cache.go
💤 Files with no reviewable changes (3)
- api/v1alpha1/release.go
- api/v1alpha1/environment.go
- api/v1alpha1/project.go
davidmdm
left a comment
There was a problem hiding this comment.
Pre-approving but check out comments for fun!
j-martin
left a comment
There was a problem hiding this comment.
Let's not forget to upgrade the code that uses this:
ce9526c to
c760df6
Compare
This PR allows
joycli (and eventuallyjoy-generator) to read catalog chart refs from eitherjoy.yamlorcatalog.yaml. The goal is to eventually only read chart refs fromcatalog.yamland remove those fromjoy.yaml.NOTE: That other catalog PR must be merged first.
Note
Medium Risk
Chart resolution affects every release render/deploy; wrong merge or missing
catalog.yamlcould change which charts resolve until catalog PR lands, though legacyjoy.yamlremains the fallback.Overview
Introduces a
Catalogv1alpha1 API (catalog.yaml) so chart refs, default chart, and optional repo/revision live in a catalog resource instead of only injoy.yaml.Config loading now reads
catalog.yaml, mergesspec.chartsinto the in-memory catalog config viaMergeCatalogResourceIntoCatalogConfig(resource refs replace the whole charts map when present; default overrides when set), then keeps validating merged charts. Stderr hints whether legacyjoy.yamlor catalog resource values were used.Scheme centralizes group/version/kind constants and adds Catalog GVK/GK; per-file kind constants were removed in favor of
scheme.go.Helm
Chart/ChartFSmove tointernal/helm;pkg/helmtype-aliases them so cache behavior stays the same.Tests cover merge edge cases and
Loadwith joy-only, catalog-only, combined priority, and invalid YAML.Reviewed by Cursor Bugbot for commit ce9526c. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
catalog.yamlresource to define chart references and a default chart.Bug Fixes
Tests