Open
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 33 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a new migrations/artifact package with a version-aware Artifact constructor and interface, a v1 schema implementation with metadata, serialization and summary behavior, and accompanying tests for version selection, serialization, summaries, and error cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new artifact migration framework, including metadata management and a versioned schema implementation for policy migrations. I have provided feedback regarding the removal of a redundant constructor call and suggested refactoring the summary count update logic in the Write method to avoid unnecessary JSON marshaling.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
migrations/artifact/v1/schema.go (1)
204-205: Avoid marshaling JSON just to refresh summary counts.Line 204 and Line 255 call
Summary()for side-effects and discard bytes. This adds unnecessary work and hides potential future marshal failures at construction time. Consider extracting count calculation into a private helper and using it in bothSummary()andWrite().♻️ Proposed refactor
func New(writer io.Writer) *artifact { doc := &artifact{ MetadataData: artifactmetadata.New(SchemaVersion, uuid.NewString(), time.Now().UTC()), Skipped: []skippedEntry{}, Namespaces: []namespaceIndexEntry{}, Actions: []actionRecord{}, SubjectConditionSets: []subjectConditionSetRecord{}, SubjectMappings: []subjectMappingRecord{}, RegisteredResources: []registeredResourceRecord{}, ObligationTriggers: []obligationTriggerRecord{}, writer: writer, } - _, _ = doc.Summary() + doc.refreshSummaryCounts() return doc } +func (a *artifact) refreshSummaryCounts() { + a.SummaryData.Counts = SummaryCounts{ + Namespaces: len(a.Namespaces), + Actions: len(a.Actions), + SubjectConditionSets: len(a.SubjectConditionSets), + SubjectMappings: len(a.SubjectMappings), + RegisteredResources: len(a.RegisteredResources), + ObligationTriggers: len(a.ObligationTriggers), + Skipped: len(a.Skipped), + } +} + func (a *artifact) Summary() ([]byte, error) { if a == nil { return nil, ErrNilArtifact } - a.SummaryData.Counts = SummaryCounts{ - Namespaces: len(a.Namespaces), - Actions: len(a.Actions), - SubjectConditionSets: len(a.SubjectConditionSets), - SubjectMappings: len(a.SubjectMappings), - RegisteredResources: len(a.RegisteredResources), - ObligationTriggers: len(a.ObligationTriggers), - Skipped: len(a.Skipped), - } + a.refreshSummaryCounts() encoded, err := json.Marshal(a.SummaryData) if err != nil { return nil, fmt.Errorf("%w: %w", ErrSummaryArtifact, err) @@ a.normalize() - if _, err := a.Summary(); err != nil { - return err - } + a.refreshSummaryCounts() encoder := json.NewEncoder(a.writer) encoder.SetIndent("", " ")Also applies to: 228-244, 254-257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@migrations/artifact/v1/schema.go` around lines 204 - 205, The Summary() method is being called for its side-effects (doc.Summary() results discarded), causing unnecessary JSON marshaling and hiding marshal errors; extract the logic that computes/refreshes summary counts into a private helper (e.g., refreshSummaryCounts or computeCounts) and have both Summary() and Write() call that helper instead of calling Summary() and discarding bytes; update Summary() to call the helper then perform marshaling, and update Write() (and any other sites that call doc.Summary() for side-effects) to call the helper directly before writing so no redundant json.Marshal occurs and marshal errors surface where appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@migrations/artifact/artifact_test.go`:
- Around line 63-69: Replace the two assert.ErrorContains calls with
require.ErrorContains so the test fails fast: after calling doc.Build() and
assigning buildErr (related to doc.Build) use require.ErrorContains(t, buildErr,
"not implemented"); and likewise after doc.Commit() and commitErr (related to
doc.Commit) use require.ErrorContains(t, commitErr, "not implemented").
In `@migrations/artifact/artifact.go`:
- Around line 23-26: The constructor currently allows ArtifactOpts with a nil
Writer which defers failure to later Write calls; update the New function (the
artifact constructor) to validate ArtifactOpts.Writer and return an immediate
error if it's nil (e.g., return nil, fmt.Errorf("writer is required")), and
propagate this change to any callers to handle the error; ensure you also add
the same nil-check wherever the constructor branches by Version (the
version-specific initialization paths referenced in the file) so the writer is
validated centrally on creation.
---
Nitpick comments:
In `@migrations/artifact/v1/schema.go`:
- Around line 204-205: The Summary() method is being called for its side-effects
(doc.Summary() results discarded), causing unnecessary JSON marshaling and
hiding marshal errors; extract the logic that computes/refreshes summary counts
into a private helper (e.g., refreshSummaryCounts or computeCounts) and have
both Summary() and Write() call that helper instead of calling Summary() and
discarding bytes; update Summary() to call the helper then perform marshaling,
and update Write() (and any other sites that call doc.Summary() for
side-effects) to call the helper directly before writing so no redundant
json.Marshal occurs and marshal errors surface where appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c871ea13-6565-4aa6-b065-85f0759bdd1c
📒 Files selected for processing (5)
migrations/artifact/artifact.gomigrations/artifact/artifact_test.gomigrations/artifact/metadata/metadata.gomigrations/artifact/v1/schema.gomigrations/artifact/v1/schema_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (2)
migrations/artifact/artifact_test.go (1)
62-68:⚠️ Potential issue | 🟡 MinorReplace
assert.ErrorContainswithrequire.ErrorContains(lint + fail-fast).Line 64 and Line 68 still violate testifylint’s
require-errorrule.Suggested patch
buildErr := doc.Build() require.Error(t, buildErr) - assert.ErrorContains(t, buildErr, "not implemented") + require.ErrorContains(t, buildErr, "not implemented") commitErr := doc.Commit() require.Error(t, commitErr) - assert.ErrorContains(t, commitErr, "not implemented") + require.ErrorContains(t, commitErr, "not implemented")#!/bin/bash # Verify no testifylint require-error violations remain in this file. rg -n '\bassert\.ErrorContains\s*\(' migrations/artifact/artifact_test.go # Expected after fix: no output🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@migrations/artifact/artifact_test.go` around lines 62 - 68, Replace the two assert.ErrorContains calls with require.ErrorContains to satisfy testifylint and fail fast: after calling doc.Build() (buildErr) and doc.Commit() (commitErr) change assert.ErrorContains(t, buildErr, "not implemented") and assert.ErrorContains(t, commitErr, "not implemented") to require.ErrorContains(t, buildErr, "not implemented") and require.ErrorContains(t, commitErr, "not implemented") respectively so the test stops immediately on failure and the lint rule is satisfied.migrations/artifact/artifact.go (1)
36-43:⚠️ Potential issue | 🟠 MajorFail fast in
NewwhenArtifactOpts.Writeris nil.Right now construction succeeds and the caller only fails later on
Write(). Validate at creation time to keep the API deterministic.Suggested patch
var ( currentSchemaVersion = semver.MustParse(CurrentSchemaVersion) ErrInvalidSchemaVersion = errors.New("invalid artifact schema version") ErrUnsupportedSchemaVersion = errors.New("unsupported artifact schema version") ErrNotImplemented = errors.New("not implemented") + ErrNilWriter = errors.New("nil artifact writer") ) @@ func New(opts ArtifactOpts) (Artifact, error) { + if opts.Writer == nil { + return nil, ErrNilWriter + } + version := opts.Version if version == nil { version = currentSchemaVersion }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@migrations/artifact/artifact.go` around lines 36 - 43, The constructor New should fail fast when ArtifactOpts.Writer is nil: add a nil-check at the start of New that returns a clear error (e.g., "missing writer") rather than allowing construction to succeed and error later in Write; update New (which currently calls newDocumentForVersion and uses opts.Writer) to validate opts.Writer != nil before calling newDocumentForVersion and return the error immediately if nil.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@migrations/artifact/artifact_test.go`:
- Around line 62-68: Replace the two assert.ErrorContains calls with
require.ErrorContains to satisfy testifylint and fail fast: after calling
doc.Build() (buildErr) and doc.Commit() (commitErr) change
assert.ErrorContains(t, buildErr, "not implemented") and assert.ErrorContains(t,
commitErr, "not implemented") to require.ErrorContains(t, buildErr, "not
implemented") and require.ErrorContains(t, commitErr, "not implemented")
respectively so the test stops immediately on failure and the lint rule is
satisfied.
In `@migrations/artifact/artifact.go`:
- Around line 36-43: The constructor New should fail fast when
ArtifactOpts.Writer is nil: add a nil-check at the start of New that returns a
clear error (e.g., "missing writer") rather than allowing construction to
succeed and error later in Write; update New (which currently calls
newDocumentForVersion and uses opts.Writer) to validate opts.Writer != nil
before calling newDocumentForVersion and return the error immediately if nil.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e1e6232-a8c5-459d-950a-a2586351dabb
📒 Files selected for processing (5)
migrations/artifact/artifact.gomigrations/artifact/artifact_test.gomigrations/artifact/metadata/metadata.gomigrations/artifact/v1/schema.gomigrations/artifact/v1/schema_test.go
✅ Files skipped from review due to trivial changes (1)
- migrations/artifact/v1/schema_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- migrations/artifact/v1/schema.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
migrations/artifact/v1/schema.go (2)
217-236: Avoid duplicated summary-count computation paths.
Summary()andupdateSummary()currently compute the same counts independently. Centralizing this to one helper reduces drift risk when fields evolve.♻️ Proposed refactor
+func (a *artifact) computeSummary() Summary { + return Summary{ + Counts: SummaryCounts{ + Namespaces: len(a.Namespaces), + Actions: len(a.Actions), + SubjectConditionSets: len(a.SubjectConditionSets), + SubjectMappings: len(a.SubjectMappings), + RegisteredResources: len(a.RegisteredResources), + ObligationTriggers: len(a.ObligationTriggers), + Skipped: len(a.Skipped), + }, + } +} + func (a *artifact) Summary() ([]byte, error) { - summary := Summary{ - Counts: SummaryCounts{ - Namespaces: len(a.Namespaces), - Actions: len(a.Actions), - SubjectConditionSets: len(a.SubjectConditionSets), - SubjectMappings: len(a.SubjectMappings), - RegisteredResources: len(a.RegisteredResources), - ObligationTriggers: len(a.ObligationTriggers), - Skipped: len(a.Skipped), - }, - } + summary := a.computeSummary() encoded, err := json.Marshal(summary) if err != nil { return nil, fmt.Errorf("%w: %w", ErrSummaryArtifact, err) } @@ func (a *artifact) updateSummary() { - a.SummaryData = Summary{ - Counts: SummaryCounts{ - Namespaces: len(a.Namespaces), - Actions: len(a.Actions), - SubjectConditionSets: len(a.SubjectConditionSets), - SubjectMappings: len(a.SubjectMappings), - RegisteredResources: len(a.RegisteredResources), - ObligationTriggers: len(a.ObligationTriggers), - Skipped: len(a.Skipped), - }, - } + a.SummaryData = a.computeSummary() }Also applies to: 254-266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@migrations/artifact/v1/schema.go` around lines 217 - 236, Summary(): and updateSummary() duplicate the same count computation; extract that logic into a single helper (e.g., a method on *artifact named computeSummaryCounts or counts) that returns SummaryCounts, then call that helper from both Summary() and updateSummary() so they share the same computation path (update the Summary() body to use the helper to build Summary{Counts: helper(...)}, and change updateSummary() to assign from the same helper).
205-211: Use a sentinel for “not implemented” lifecycle methods.Current string-only errors are fine for now, but a sentinel lets callers use
errors.Is(...)reliably when Build/Commit are intentionally unavailable.♻️ Proposed refactor
var ( ErrNilWriter = errors.New("nil writer") ErrWriteArtifact = errors.New("write artifact") ErrSummaryArtifact = errors.New("summary artifact") + ErrNotImplemented = errors.New("not implemented") ) @@ func (a *artifact) Build() error { - return fmt.Errorf("artifact build for schema %s: not implemented", SchemaVersion) + return fmt.Errorf("artifact build for schema %s: %w", SchemaVersion, ErrNotImplemented) } func (a *artifact) Commit() error { - return fmt.Errorf("artifact commit for schema %s: not implemented", SchemaVersion) + return fmt.Errorf("artifact commit for schema %s: %w", SchemaVersion, ErrNotImplemented) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@migrations/artifact/v1/schema.go` around lines 205 - 211, Replace the current string-only errors in artifact.Build and artifact.Commit with a package-level sentinel error so callers can reliably use errors.Is; define a var like ErrNotImplemented = errors.New("not implemented") in the same package and return wrapped errors from Build and Commit (e.g. fmt.Errorf("artifact build for schema %s: %w", SchemaVersion, ErrNotImplemented) and similarly for Commit) so the message retains context while allowing errors.Is to detect the sentinel.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@migrations/artifact/v1/schema.go`:
- Around line 217-236: Summary(): and updateSummary() duplicate the same count
computation; extract that logic into a single helper (e.g., a method on
*artifact named computeSummaryCounts or counts) that returns SummaryCounts, then
call that helper from both Summary() and updateSummary() so they share the same
computation path (update the Summary() body to use the helper to build
Summary{Counts: helper(...)}, and change updateSummary() to assign from the same
helper).
- Around line 205-211: Replace the current string-only errors in artifact.Build
and artifact.Commit with a package-level sentinel error so callers can reliably
use errors.Is; define a var like ErrNotImplemented = errors.New("not
implemented") in the same package and return wrapped errors from Build and
Commit (e.g. fmt.Errorf("artifact build for schema %s: %w", SchemaVersion,
ErrNotImplemented) and similarly for Commit) so the message retains context
while allowing errors.Is to detect the sentinel.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91494f00-caf3-47f1-9920-5a9d9cfe4bf0
📒 Files selected for processing (2)
migrations/artifact/v1/schema.gomigrations/artifact/v1/schema_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- migrations/artifact/v1/schema_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Step 1: Artifact Model
Summary
This PR implements step 1 of the policy migration plan.
It adds the initial versioned artifact model for migrate policy-graph output under migrations/artifact, with the current schema implemented as v1.0.0.
Included
Not Included
Testing
go test ./migrations/...
Summary by CodeRabbit
New Features
Tests