feat: Playbook panels#2614
Conversation
|
Warning Rate limit exceeded@adityathebe has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 16 minutes and 17 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (9)
WalkthroughAdds a new "playbooks" panel type across API types, CRD/schema, and runtime: PlaybooksPanelConfig + selector, view validation updated to allow playbooks-only views, view runner executes playbooks selectors with RLS-backed SQL, fixtures added, and module deps updated. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant ViewRunner as View Runner
participant SelectorSvc as Resource Selector Resolver
participant Auth as auth.WithRLS
participant DB as Database
User->>ViewRunner: request render view
ViewRunner->>ViewRunner: iterate panels
alt panel.type == "playbooks"
ViewRunner->>SelectorSvc: resolve selector -> matching IDs
SelectorSvc-->>ViewRunner: list of IDs
ViewRunner->>Auth: build SQL with ID filters + RLS context
Auth->>DB: execute parameterized SELECT WHERE id IN (...)
DB-->>Auth: rows
Auth-->>ViewRunner: result rows
else other panel types
ViewRunner->>Auth: execute panel.Query via RunSQL
Auth->>DB: execute SQL
DB-->>Auth: rows
Auth-->>ViewRunner: result rows
end
ViewRunner-->>User: composed view results
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
views/run.go (1)
150-190: Consider documenting or making the playbook selector limit configurable.The hardcoded limit of 200 on line 160 may silently truncate results if a selector matches more playbooks. Consider either:
- Documenting this limit in the CRD/API
- Making it configurable via the panel config
- Using a larger default or warning when the limit is reached
Otherwise, the implementation is sound - RLS protection is correctly applied, and the squirrel query builder usage is appropriate.
+// playbookSelectorLimit is the maximum number of playbooks to fetch when filtering by selector +const playbookSelectorLimit = 200 + ... - ids, err := query.FindPlaybookIDsByResourceSelector(ctx, 200, panel.Playbooks.Selector) + ids, err := query.FindPlaybookIDsByResourceSelector(ctx, playbookSelectorLimit, panel.Playbooks.Selector)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
api/v1/view_types.go(3 hunks)api/view_panel.go(4 hunks)api/zz_generated.deepcopy.go(2 hunks)config/crds/mission-control.flanksource.com_views.yaml(5 hunks)config/schemas/view.schema.json(3 hunks)fixtures/views/panels/playbook-run.yaml(1 hunks)go.mod(2 hunks)views/run.go(4 hunks)views/table.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Use context from github.com/flanksource/duty/context.Context to access db (ctx.DB()), properties (ctx.Properties()), and logger (ctx.Logger)
If a file needs to use both context and duty's context, alias context to 'gocontext'
Preferanyoverinterface{}in Go type declarations
Usectx.Oops()to craft new errors with error codes fromgithub.com/flanksource/duty/apias tags
UsedutyAPI.WriteError(c, err)in HTTP handlers to return errors with proper HTTP status code mapping based on error codes
For validation errors with no underlying error in HTTP handlers, usedutyAPI.Errorf(dutyAPI.EINVALID, "message")
For wrapping database/internal errors in HTTP handlers, usectx.Oops().Wrap(err)orctx.Oops().Wrapf(err, "context")
For permission errors in HTTP handlers, usectx.Oops().Code(dutyAPI.EFORBIDDEN).Errorf("message")
Useduty.Now()instead oftime.Now()for database timestamps and soft deletes
Only add comments if really necessary. Do not add comments that simply explain the code. Exception: comments about functions are considered good practice in Go even if they are self-explanatory
Files:
views/run.goapi/v1/view_types.goviews/table.goapi/zz_generated.deepcopy.goapi/view_panel.go
api/v1/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
CRD definitions must be located in the api/v1 directory
Files:
api/v1/view_types.go
**/*zz_generated.deepcopy.go
📄 CodeRabbit inference engine (AGENTS.md)
Never manually update
zz_generated.deepcopy.gofiles. Usemake generateinstead
Files:
api/zz_generated.deepcopy.go
🧠 Learnings (4)
📚 Learning: 2025-11-24T18:35:17.129Z
Learnt from: CR
Repo: flanksource/mission-control PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T18:35:17.129Z
Learning: Applies to **/*.go : Use `duty.Now()` instead of `time.Now()` for database timestamps and soft deletes
Applied to files:
views/run.gogo.mod
📚 Learning: 2025-11-24T18:35:17.129Z
Learnt from: CR
Repo: flanksource/mission-control PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T18:35:17.129Z
Learning: Applies to **/*.go : Use context from github.com/flanksource/duty/context.Context to access db (ctx.DB()), properties (ctx.Properties()), and logger (ctx.Logger)
Applied to files:
views/run.gogo.modapi/v1/view_types.go
📚 Learning: 2025-11-24T18:35:17.129Z
Learnt from: CR
Repo: flanksource/mission-control PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T18:35:17.129Z
Learning: Migrations are handled by the external package `github.com/flanksource/duty` using Atlas-go
Applied to files:
views/run.gogo.modapi/v1/view_types.go
📚 Learning: 2025-11-24T18:35:17.129Z
Learnt from: CR
Repo: flanksource/mission-control PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T18:35:17.129Z
Learning: Applies to **/*.go : Use `ctx.Oops()` to craft new errors with error codes from `github.com/flanksource/duty/api` as tags
Applied to files:
go.modapi/v1/view_types.go
🧬 Code graph analysis (3)
views/run.go (2)
api/view_panel.go (1)
PanelTypePlaybooks(20-20)auth/rls.go (1)
WithRLS(55-77)
api/v1/view_types.go (1)
api/view_panel.go (2)
PanelDef(32-37)PanelTypePlaybooks(20-20)
api/zz_generated.deepcopy.go (1)
api/view_panel.go (1)
PlaybooksPanelConfig(40-43)
🔇 Additional comments (19)
go.mod (1)
15-15: LGTM! Dependency updates align with the feature.The duty library bump and promoting squirrel to a direct dependency are appropriate for the Playbooks panel feature.
Also applies to: 41-41
api/zz_generated.deepcopy.go (1)
332-336: Verify this file was regenerated usingmake generate.Per coding guidelines,
zz_generated.deepcopy.gofiles should never be manually updated. The changes look correct for the newPlaybooksPanelConfigtype, but please confirm this file was regenerated viamake generaterather than manually edited.Also applies to: 401-415
api/v1/view_types.go (3)
14-14: LGTM!The
loimport is used appropriately for filtering panels.
66-66: LGTM!The XValidation rule correctly allows views with playbooks panels to bypass the sections/queries requirement.
138-145: Clarify: Does a view combining playbooks panels with sections require queries?The validation logic at line 141 explicitly excludes views with sections when determining if a view has only playbooks panels. This means views combining playbooks panels and sections would not return early and would fall through to subsequent validation. Confirm whether this combination (playbooks panels + sections without queries) is intentionally invalid or if the condition should be adjusted.
config/schemas/view.schema.json (2)
480-492: LGTM!The schema correctly adds the
playbooksproperty and removesqueryfrom required fields, allowing playbooks panels that don't require a SQL query.
550-561: LGTM!The
PlaybooksPanelConfigdefinition is correctly structured withselectoras a required property referencing theResourceSelectortype.api/view_panel.go (3)
30-31: LGTM!The XValidation rules correctly enforce:
playbooksconfig is only allowed whentype == 'playbooks'queryis required for all non-playbooks panel types
39-43: LGTM!The
PlaybooksPanelConfigstruct is well-defined with the+kubebuilder:object:generate=truemarker for deepcopy generation and uses the appropriatetypes.ResourceSelectorfor the selector field.
71-73: LGTM!The
Playbooksfield follows the same optional pointer pattern as other panel configurations (Gauge, Piechart, etc.), maintaining consistency.views/table.go (2)
292-292: LGTM!The constant rename to
variableMaxResultForValueFromis consistent with the definition inviews/run.goand improves clarity about the constant's purpose.
537-537: LGTM!Consistent usage of the renamed constant.
fixtures/views/panels/playbook-run.yaml (1)
1-16: LGTM!This fixture correctly demonstrates a playbooks-only view with the new panel type. The structure aligns with the CRD schema changes and validation rules that allow views containing only playbooks panels without traditional queries.
views/run.go (3)
28-29: LGTM!Good rename with a clear descriptive comment explaining the constant's purpose.
122-122: LGTM!Correct logic update: SQLite is only needed when there are both panels and queries (for panel SQL queries against query results), or when there's a merge query. Playbooks panels query Postgres directly and don't need the in-memory SQLite database.
177-183: Cannot verify function existence due to repository access limitations.The review comment requests verification of
query.FindPlaybookIDsByResourceSelectorfrom the duty package; however, the provided code snippet at lines 177-183 does not show a call to this function—it shows adataquery.RunSQLcall withinauth.WithRLS. This discrepancy suggests the review comment may reference different code than what was provided. Verify that the function signature and location are correctly identified before proceeding with this review.config/crds/mission-control.flanksource.com_views.yaml (3)
493-558: LGTM!The playbooks panel configuration schema is well-defined. It correctly:
- Reuses the ResourceSelector type for consistency
- Marks
selectoras required to ensure valid playbook targeting- Follows the same pattern as other resource selectors in the CRD
594-597: LGTM!The validation rules are correctly structured:
- Line 594-595: Ensures
playbooksconfig is only present whentype == 'playbooks'- Line 596-597: Makes
queryrequired for all panel types exceptplaybooks, since playbooks panels use their selector instead
1420-1426: LGTM!The top-level validation correctly allows views that contain only playbooks panels without requiring traditional sections or queries. This enables the use case demonstrated in the fixture file.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
views/run.go (1)
153-183: Playbooks panel implementation looks correct.The implementation properly:
- Builds the query with explicit column selection
- Filters by selector when provided (using
FindPlaybookIDsByResourceSelector)- Executes within RLS context for proper access control
- Uses parameterized queries with Dollar placeholders
One consideration: The hardcoded limit of
200on line 160 forFindPlaybookIDsByResourceSelectormay be worth making configurable via the selector'sLimitfield if that's supported.if panel.Playbooks != nil && !panel.Playbooks.Selector.IsEmpty() { - ids, err := query.FindPlaybookIDsByResourceSelector(ctx, 200, panel.Playbooks.Selector) + limit := 200 + if panel.Playbooks.Selector.Limit > 0 { + limit = panel.Playbooks.Selector.Limit + } + ids, err := query.FindPlaybookIDsByResourceSelector(ctx, limit, panel.Playbooks.Selector)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
api/v1/view_types.go(3 hunks)api/view_panel.go(4 hunks)api/zz_generated.deepcopy.go(2 hunks)config/crds/mission-control.flanksource.com_views.yaml(5 hunks)config/schemas/view.schema.json(3 hunks)fixtures/views/panels/playbook-run.yaml(1 hunks)go.mod(2 hunks)views/run.go(4 hunks)views/table.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Use context from github.com/flanksource/duty/context.Context to access db (ctx.DB()), properties (ctx.Properties()), and logger (ctx.Logger)
If a file needs to use both context and duty's context, alias context to 'gocontext'
Preferanyoverinterface{}in Go type declarations
Usectx.Oops()to craft new errors with error codes fromgithub.com/flanksource/duty/apias tags
UsedutyAPI.WriteError(c, err)in HTTP handlers to return errors with proper HTTP status code mapping based on error codes
For validation errors with no underlying error in HTTP handlers, usedutyAPI.Errorf(dutyAPI.EINVALID, "message")
For wrapping database/internal errors in HTTP handlers, usectx.Oops().Wrap(err)orctx.Oops().Wrapf(err, "context")
For permission errors in HTTP handlers, usectx.Oops().Code(dutyAPI.EFORBIDDEN).Errorf("message")
Useduty.Now()instead oftime.Now()for database timestamps and soft deletes
Only add comments if really necessary. Do not add comments that simply explain the code. Exception: comments about functions are considered good practice in Go even if they are self-explanatory
Files:
views/run.goapi/v1/view_types.goapi/zz_generated.deepcopy.goviews/table.goapi/view_panel.go
api/v1/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
CRD definitions must be located in the api/v1 directory
Files:
api/v1/view_types.go
**/*zz_generated.deepcopy.go
📄 CodeRabbit inference engine (AGENTS.md)
Never manually update
zz_generated.deepcopy.gofiles. Usemake generateinstead
Files:
api/zz_generated.deepcopy.go
🧠 Learnings (4)
📚 Learning: 2025-11-24T18:35:17.129Z
Learnt from: CR
Repo: flanksource/mission-control PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T18:35:17.129Z
Learning: Applies to **/*.go : Use `duty.Now()` instead of `time.Now()` for database timestamps and soft deletes
Applied to files:
views/run.gogo.mod
📚 Learning: 2025-11-24T18:35:17.129Z
Learnt from: CR
Repo: flanksource/mission-control PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T18:35:17.129Z
Learning: Applies to **/*.go : Use context from github.com/flanksource/duty/context.Context to access db (ctx.DB()), properties (ctx.Properties()), and logger (ctx.Logger)
Applied to files:
views/run.goapi/v1/view_types.gogo.mod
📚 Learning: 2025-11-24T18:35:17.129Z
Learnt from: CR
Repo: flanksource/mission-control PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T18:35:17.129Z
Learning: Migrations are handled by the external package `github.com/flanksource/duty` using Atlas-go
Applied to files:
views/run.goapi/v1/view_types.gogo.mod
📚 Learning: 2025-11-24T18:35:17.129Z
Learnt from: CR
Repo: flanksource/mission-control PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T18:35:17.129Z
Learning: Applies to **/*.go : Use `ctx.Oops()` to craft new errors with error codes from `github.com/flanksource/duty/api` as tags
Applied to files:
api/v1/view_types.gogo.mod
🧬 Code graph analysis (3)
views/run.go (2)
api/view_panel.go (1)
PanelTypePlaybooks(20-20)auth/rls.go (1)
WithRLS(55-77)
api/v1/view_types.go (1)
api/view_panel.go (2)
PanelDef(32-37)PanelTypePlaybooks(20-20)
api/zz_generated.deepcopy.go (1)
api/view_panel.go (1)
PlaybooksPanelConfig(40-43)
🔇 Additional comments (15)
go.mod (1)
15-15: LGTM!Dependency updates are aligned with the new playbooks panel feature. The
squirrellibrary is appropriately promoted to a direct dependency since it's now used for SQL query construction in the playbooks panel logic.Also applies to: 41-41
api/zz_generated.deepcopy.go (1)
1-3: Autogenerated file - no manual changes required.This file is correctly auto-generated by controller-gen. The deepcopy functions for
PlaybooksPanelConfigand thePlaybooksfield inPanelMetaare properly generated. As per coding guidelines, ensure this file was generated usingmake generateand not manually edited.views/table.go (1)
292-292: LGTM!The constant rename from
valueFromMaxResultstovariableMaxResultForValueFromimproves clarity by better describing its purpose for templating variablevalueFromqueries.Also applies to: 537-537
fixtures/views/panels/playbook-run.yaml (1)
1-16: LGTM!Good fixture demonstrating a playbooks-only view. The selector correctly uses comma-separated names to match multiple playbooks (
loki-logs,kubernetes-scale).api/v1/view_types.go (2)
66-66: LGTM!The XValidation rule correctly extends the validation to accept views that have only playbooks panels as a valid configuration alongside views with sections or queries.
138-145: LGTM!The validation logic for playbooks-only views is well-implemented. It correctly filters non-playbook panels and allows views that contain only playbooks panels to bypass the query requirement.
config/schemas/view.schema.json (2)
480-492: LGTM!Schema correctly adds the
playbooksproperty and relaxes therequiredconstraint to removequery, since playbooks panels don't require a query field.
550-561: LGTM!
PlaybooksPanelConfigis correctly defined withselectoras a required field referencingResourceSelector.api/view_panel.go (2)
30-31: LGTM!The XValidation rules are correctly implemented:
- Playbooks config is only allowed for playbooks panel type
- Query is required for all panel types except playbooks
39-43: LGTM!
PlaybooksPanelConfigis properly defined with aSelectorfield that allows filtering playbooks by resource selector criteria.views/run.go (2)
122-122: LGTM!The logic correctly avoids creating an in-memory SQLite database when not needed. Playbooks panels query PostgreSQL directly, so SQLite is only needed when there are both regular panels and queries defined.
185-190: LGTM!The default case correctly handles non-playbooks panels using the existing SQLite-based query execution.
config/crds/mission-control.flanksource.com_views.yaml (3)
493-558: Playbooks panel schema follows established patterns with comprehensive selector fields.The playbooks panel configuration reuses the standard resource selector pattern used elsewhere in the CRD (e.g.,
configTabindisplay.plugins). The requiredselectorfield appropriately constrains the panel to require explicit filtering criteria for playbook selection. All standard filter fields (agent, cache, fieldSelector, health, id, labelSelector, limit, name, namespace, scope, search, statuses, tagSelector, types) are available.Verify that the
required: [selector]constraint is intentional—i.e., there is no use case to render all playbooks without a selector. If a "show all" mode is needed, the required constraint should be removed.
594-597: Validation rules correctly enforce playbooks panel constraints.Line 596–597 correctly inverts the query requirement:
queryis mandatory for all panel types except playbooks (where it is not required). This aligns with the playbooks panel's self-contained selector-based data source. The constraint on line 594–595 properly prevents playbooks config from appearing on non-playbooks panels.
1422-1423: Global view validation relaxed to permit playbooks-only views.The validation rule now allows a view to satisfy the "content must be defined" constraint by including a playbooks panel, alongside sections or queries. This enables valid views with playbooks as their sole content source, which is consistent with the feature design.
adityathebe
force-pushed
the
feat/playbook-panel
branch
from
50eba2d to
29a4d21
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
views/run.go (1)
185-190: Consider adding a nil check forsqliteCtx.If a non-playbooks panel is processed but
sqliteCtxis nil (e.g., due to validation being bypassed or future code changes),dataquery.RunSQL(sqliteCtx, panel.Query)could panic.default: + if sqliteCtx == nil { + return nil, fmt.Errorf("failed to execute panel '%s': no SQL context available", panel.Name) + } rows, err = dataquery.RunSQL(sqliteCtx, panel.Query) if err != nil { return nil, fmt.Errorf("failed to execute panel '%s': %w", panel.Name, err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
api/v1/view_types.go(3 hunks)api/view_panel.go(4 hunks)api/zz_generated.deepcopy.go(2 hunks)config/crds/mission-control.flanksource.com_views.yaml(5 hunks)config/schemas/view.schema.json(3 hunks)fixtures/views/panels/playbook-run.yaml(1 hunks)go.mod(2 hunks)views/run.go(4 hunks)views/table.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- api/zz_generated.deepcopy.go
- views/table.go
- fixtures/views/panels/playbook-run.yaml
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Use context from github.com/flanksource/duty/context.Context to access db (ctx.DB()), properties (ctx.Properties()), and logger (ctx.Logger)
If a file needs to use both context and duty's context, alias context to 'gocontext'
Preferanyoverinterface{}in Go type declarations
Usectx.Oops()to craft new errors with error codes fromgithub.com/flanksource/duty/apias tags
UsedutyAPI.WriteError(c, err)in HTTP handlers to return errors with proper HTTP status code mapping based on error codes
For validation errors with no underlying error in HTTP handlers, usedutyAPI.Errorf(dutyAPI.EINVALID, "message")
For wrapping database/internal errors in HTTP handlers, usectx.Oops().Wrap(err)orctx.Oops().Wrapf(err, "context")
For permission errors in HTTP handlers, usectx.Oops().Code(dutyAPI.EFORBIDDEN).Errorf("message")
Useduty.Now()instead oftime.Now()for database timestamps and soft deletes
Only add comments if really necessary. Do not add comments that simply explain the code. Exception: comments about functions are considered good practice in Go even if they are self-explanatory
Files:
views/run.goapi/view_panel.goapi/v1/view_types.go
api/v1/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
CRD definitions must be located in the api/v1 directory
Files:
api/v1/view_types.go
🧠 Learnings (4)
📚 Learning: 2025-11-24T18:35:17.129Z
Learnt from: CR
Repo: flanksource/mission-control PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T18:35:17.129Z
Learning: Applies to **/*.go : Use `duty.Now()` instead of `time.Now()` for database timestamps and soft deletes
Applied to files:
views/run.gogo.mod
📚 Learning: 2025-11-24T18:35:17.129Z
Learnt from: CR
Repo: flanksource/mission-control PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T18:35:17.129Z
Learning: Applies to **/*.go : Use context from github.com/flanksource/duty/context.Context to access db (ctx.DB()), properties (ctx.Properties()), and logger (ctx.Logger)
Applied to files:
views/run.gogo.modapi/v1/view_types.go
📚 Learning: 2025-11-24T18:35:17.129Z
Learnt from: CR
Repo: flanksource/mission-control PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T18:35:17.129Z
Learning: Migrations are handled by the external package `github.com/flanksource/duty` using Atlas-go
Applied to files:
views/run.gogo.modapi/v1/view_types.go
📚 Learning: 2025-11-24T18:35:17.129Z
Learnt from: CR
Repo: flanksource/mission-control PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T18:35:17.129Z
Learning: Applies to **/*.go : Use `ctx.Oops()` to craft new errors with error codes from `github.com/flanksource/duty/api` as tags
Applied to files:
go.modapi/v1/view_types.go
🧬 Code graph analysis (2)
views/run.go (2)
api/view_panel.go (1)
PanelTypePlaybooks(18-18)auth/rls.go (1)
WithRLS(55-77)
api/v1/view_types.go (1)
api/view_panel.go (2)
PanelDef(33-38)PanelTypePlaybooks(18-18)
🔇 Additional comments (15)
go.mod (1)
15-15: LGTM!The dependency updates are appropriate:
dutypatch bump andsquirrelpromotion to direct dependency align with the new Playbooks panel SQL query construction inviews/run.go.Also applies to: 41-41
api/v1/view_types.go (3)
14-14: LGTM!Import addition for
lopackage is appropriate for the filtering operation below.
66-66: LGTM!The updated validation rule correctly allows views that have playbooks panels as a valid alternative to sections or queries. The CEL expression
self.panels.exists(p, p.type == 'playbooks')ensures at least one playbooks panel must exist for this path.
138-145: LGTM!The validation logic correctly identifies playbooks-only views and bypasses the query requirement. The filtering approach using
lo.Filteris clean, and the condition properly checks that no other view components (columns, queries, sections, non-playbook panels) are present.views/run.go (2)
9-9: LGTM!New imports are appropriate:
squirrelfor SQL query building,uuidfor ID handling, andauthfor RLS enforcement.Also applies to: 19-19, 25-25
153-190: Code implementation is sound with proper security controls.The playbooks panel handling is well-structured:
- Uses parameterized queries via squirrel to prevent SQL injection
- Enforces RLS via
auth.WithRLSfor proper access control- Filters out soft-deleted records with
deleted_at IS NULLTwo observations to be aware of:
Hardcoded limit (Line 160): The limit of 200 for
FindPlaybookIDsByResourceSelectoris hardcoded in the call. This appears to be a fixed parameter for this use case, but verify if it should be exposed as a configuration option.Empty selector behavior (Line 159): When
panel.Playbooksis nil or the selector is empty, all non-deleted playbooks are returned without filtering. Confirm this is the intended behavior for this panel type.config/schemas/view.schema.json (2)
480-492: LGTM!The schema changes are correct:
- Added
playbooksproperty referencingPlaybooksPanelConfig- Removed
queryfrom required fields (appropriate since playbooks panels don't use queries)- Required fields now only include
nameandtype
550-561: LGTM!The
PlaybooksPanelConfigdefinition correctly requires aselectorfield of typeResourceSelector. This aligns with the Go type definition inapi/view_panel.go.api/view_panel.go (5)
5-5: LGTM!Import addition for
typespackage is necessary for theResourceSelectortype used inPlaybooksPanelConfig.
13-22: LGTM!Panel type constants are well-organized alphabetically with the new
PanelTypePlaybooksadded.
31-32: LGTM!The validation rules correctly enforce:
playbooksconfig is only allowed when type isplaybooksqueryis required for all non-playbooks panel typesThis ensures proper panel configuration based on type.
40-44: LGTM!The
PlaybooksPanelConfigstruct is well-defined with proper kubebuilder marker for code generation. TheSelectorfield correctly usestypes.ResourceSelectorfor flexible playbook filtering.
54-56: LGTM!The enum validation is updated to include
playbooks, andPanelMetacorrectly includes the optionalPlaybooksconfiguration pointer, consistent with other panel-specific configs.Also applies to: 72-74
config/crds/mission-control.flanksource.com_views.yaml (2)
1420-1426: Global view validation correctly updated for playbooks-only views.Line 1422–1423 now allows views to be valid when they contain playbooks panels, removing the hard requirement for sections or queries. This enables the new use case of playbooks-only views.
The logic correctly allows
(sections OR queries OR playbooks_panel), which properly gates valid view configurations.
493-598: Schema integration for playbooks panel is complete and properly implemented.The playbooks panel type is correctly integrated into the CRD with:
- A well-defined
PlaybooksPanelConfigwith a requiredselector(lines 493–558)- Consistent validation enforcement: playbooks config only allowed for
type: playbooks(lines 594–595), and query required only for non-playbooks types (lines 596–597)- The panel type enum properly updated (line 577)
The backend implementation is verified and working:
- Panel execution handler in
views/run.go(lines 153–170) usesFindPlaybookIDsByResourceSelectorto fetch matching playbooks- Selector-based filtering properly applied with error handling for empty or failed selector queries
- RLS filtering integrated through the context-aware selector query
- Edge cases handled: empty selectors skip the WHERE clause, empty results return an empty slice, and spec parsing errors are logged without interrupting execution
- Permissions tests confirm RLS behavior with playbooks
adityathebe
force-pushed
the
feat/playbook-panel
branch
from
29a4d21 to
8063eac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
views/run.go (1)
160-160: Consider making the playbook selector limit configurable or documented.The hardcoded limit of
200inFindPlaybookIDsByResourceSelectorcould silently truncate results if more playbooks match the selector. Consider either:
- Making this configurable via the
PlaybooksPanelConfig- Adding a log warning when the limit is reached
- Documenting this limitation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/v1/view_types.go(3 hunks)api/view_panel.go(4 hunks)api/zz_generated.deepcopy.go(2 hunks)config/crds/mission-control.flanksource.com_views.yaml(5 hunks)config/schemas/view.schema.json(3 hunks)fixtures/views/panels/playbook-run.yaml(1 hunks)go.mod(1 hunks)views/run.go(5 hunks)views/table.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- views/table.go
- go.mod
- fixtures/views/panels/playbook-run.yaml
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Use context from github.com/flanksource/duty/context.Context to access db (ctx.DB()), properties (ctx.Properties()), and logger (ctx.Logger)
If a file needs to use both context and duty's context, alias context to 'gocontext'
Preferanyoverinterface{}in Go type declarations
Usectx.Oops()to craft new errors with error codes fromgithub.com/flanksource/duty/apias tags
UsedutyAPI.WriteError(c, err)in HTTP handlers to return errors with proper HTTP status code mapping based on error codes
For validation errors with no underlying error in HTTP handlers, usedutyAPI.Errorf(dutyAPI.EINVALID, "message")
For wrapping database/internal errors in HTTP handlers, usectx.Oops().Wrap(err)orctx.Oops().Wrapf(err, "context")
For permission errors in HTTP handlers, usectx.Oops().Code(dutyAPI.EFORBIDDEN).Errorf("message")
Useduty.Now()instead oftime.Now()for database timestamps and soft deletes
Only add comments if really necessary. Do not add comments that simply explain the code. Exception: comments about functions are considered good practice in Go even if they are self-explanatory
Files:
views/run.goapi/view_panel.goapi/zz_generated.deepcopy.goapi/v1/view_types.go
**/*zz_generated.deepcopy.go
📄 CodeRabbit inference engine (AGENTS.md)
Never manually update
zz_generated.deepcopy.gofiles. Usemake generateinstead
Files:
api/zz_generated.deepcopy.go
api/v1/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
CRD definitions must be located in the api/v1 directory
Files:
api/v1/view_types.go
🧠 Learnings (4)
📚 Learning: 2025-11-24T18:35:17.129Z
Learnt from: CR
Repo: flanksource/mission-control PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T18:35:17.129Z
Learning: Applies to **/*.go : Use `duty.Now()` instead of `time.Now()` for database timestamps and soft deletes
Applied to files:
views/run.go
📚 Learning: 2025-11-24T18:35:17.129Z
Learnt from: CR
Repo: flanksource/mission-control PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T18:35:17.129Z
Learning: Applies to **/*.go : Use context from github.com/flanksource/duty/context.Context to access db (ctx.DB()), properties (ctx.Properties()), and logger (ctx.Logger)
Applied to files:
views/run.goapi/v1/view_types.go
📚 Learning: 2025-11-24T18:35:17.129Z
Learnt from: CR
Repo: flanksource/mission-control PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T18:35:17.129Z
Learning: Migrations are handled by the external package `github.com/flanksource/duty` using Atlas-go
Applied to files:
views/run.goapi/v1/view_types.go
📚 Learning: 2025-11-24T18:35:17.129Z
Learnt from: CR
Repo: flanksource/mission-control PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T18:35:17.129Z
Learning: Applies to **/*.go : Use `ctx.Oops()` to craft new errors with error codes from `github.com/flanksource/duty/api` as tags
Applied to files:
api/v1/view_types.go
🧬 Code graph analysis (2)
views/run.go (2)
api/view_panel.go (1)
PanelTypePlaybooks(18-18)auth/rls.go (1)
WithRLS(55-77)
api/zz_generated.deepcopy.go (1)
api/view_panel.go (1)
PlaybooksPanelConfig(41-44)
🔇 Additional comments (13)
api/zz_generated.deepcopy.go (1)
1-415: Auto-generated file looks correct.The deep copy implementations for
PlaybooksPanelConfigand thePlaybooksfield inPanelMetaare properly generated. TheSelectorfield is correctly deep-copied usingin.Selector.DeepCopyInto(&out.Selector).As per coding guidelines, ensure this file is regenerated via
make generaterather than manually edited.api/v1/view_types.go (2)
66-66: XValidation rule correctly updated for playbooks panel support.The validation rule now allows views with only playbooks panels to bypass the sections/queries requirement. The CEL expression
self.panels.exists(p, p.type == 'playbooks')properly checks for the presence of playbooks-type panels.
138-145: Go-side validation correctly mirrors the XValidation rule.The logic properly allows playbooks-only views to skip the query requirement:
- Filters out playbook panels using
lo.Filter- Checks if the view contains only playbook panels (no columns, queries, sections, or other panel types)
- Returns early without requiring queries in this case
This aligns with the CRD validation and ensures consistent behavior.
config/schemas/view.schema.json (2)
480-492: Schema correctly adds playbooks panel support.The
PanelDefschema properly:
- Adds the optional
playbooksproperty referencingPlaybooksPanelConfig- Removes
queryfrom the required fields array (now onlynameandtypeare required)This aligns with the Go validation rules where
queryis conditionally required based on panel type.
550-561: PlaybooksPanelConfig schema definition is correct.The schema properly defines:
selectorproperty referencingResourceSelectorselectoras a required field- No additional properties allowed
This matches the Go struct definition in
api/view_panel.go.views/run.go (2)
122-122: Good fix for the nilsqliteCtxissue.The condition at line 139 now requires
len(view.Spec.Queries) > 0in addition to panels being present. This ensuressqliteCtxis always initialized (vianeedsSQLat line 122) before reaching the default panel case that callsdataquery.RunSQL(sqliteCtx, panel.Query).This addresses the critical issue flagged in the previous review.
Also applies to: 139-139
153-183: Playbooks panel implementation is well-structured.The implementation correctly:
- Builds a parameterized query using squirrel with
sq.Dollarplaceholder format- Filters out soft-deleted playbooks with
WHERE deleted_at IS NULL- Applies RLS via
auth.WithRLSfor proper access control- Wraps errors with panel context for debugging
api/view_panel.go (3)
31-32: XValidation rules correctly enforce panel type constraints.The two validation rules work together properly:
- Line 31: Prevents
playbooksconfig on non-playbooks panel types- Line 32: Requires
queryfor all panel types exceptplaybooksThis ensures type-specific configuration is enforced at the CRD level.
40-44:PlaybooksPanelConfigstruct is well-defined.The struct correctly:
- Uses
types.ResourceSelectorfor flexible playbook selection- Has proper JSON/YAML tags
- Includes the
+kubebuilder:object:generate=trueannotation for deep copy generation- Has a descriptive comment explaining the selector's purpose
54-56: Enum validation updated correctly.The
+kubebuilder:validation:Enumannotation now includesplaybooksin the list of valid panel types, aligning with the newPanelTypePlaybooksconstant.config/crds/mission-control.flanksource.com_views.yaml (3)
493-558: Playbooks selector schema follows established patterns.The selector definition correctly mirrors existing selector schemas (e.g.,
configTab.selector) with consistent field names, types, and documentation. The requiredselectorfield is appropriately marked.
594-597: CEL validation rules correctly enforce playbooks panel constraints.The two validation rules properly enforce:
- Playbooks config only allowed for
type == 'playbooks'- Query field required for all non-playbooks panel types
The CEL syntax is correct and aligns with existing validation patterns in the file.
567-581: Verify that removingqueryfrom required fields doesn't break downstream code.The change removes
queryfrom the panel's required fields list, allowing playbooks panels to omit the query field while other panel types enforce it via validation rules. This is correct in principle, but needs verification across the codebase to ensure no code paths assumequeryis always present on panels.Key areas to verify:
- Code that accesses panel.query without null/existence checks
- Validation rules that enforce query for non-playbook panel types
- Playbooks panel handling to confirm it correctly operates without a query field
adityathebe
force-pushed
the
feat/playbook-panel
branch
from
3f54544 to
3144d12
Compare
1 participant
resolves: #2294
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.