fix: honor inherited DAG type during validation#1981
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe pull request adds base config path propagation through the DAG store initialization chain. When a base config file is configured, DAG specs loaded through the store now inherit configuration (such as type) from that base config, and conditional logic bypasses file caching and index loading in favor of direct filesystem scanning when a base config path is set. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Frontend API
participant Store as DAG Store
participant FS as Filesystem
participant Spec as Spec Loader
Client->>API: Create/Validate DAG (steps only)
API->>Store: LoadSpec(dagYAML)
activate Store
Store->>Store: defaultLoadOptions()
Note over Store: Check if baseConfigPath set
alt Base Config Path Set
Store->>FS: Read base config file
FS-->>Store: Base config content
Store->>Spec: LoadSpec with WithBaseConfig()
else No Base Config Path
Store->>Spec: LoadSpec (standard options)
end
deactivate Store
Spec->>Spec: Merge base config into DAG spec
Spec-->>Store: Loaded DAG (type inherited)
Store-->>API: DAG with type: graph
API-->>Client: Validation succeeds / DAG created
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/test/helper.go (1)
246-251: Initialize the base config in the helper before constructing the store.Production startup creates the default base config in
internal/cmd/context.go:392-404, but this helper only forwardscfg.Paths.BaseConfigintofiledag.New(...). Mirroring that initialization here would keep helper-backed tests aligned with real server/scheduler startup semantics and avoid depending on an uninitializedbase_configpath.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/test/helper.go` around lines 246 - 251, Before calling filedag.New to create dagStore, ensure the default base config is created at cfg.Paths.BaseConfig exactly as done in production startup; replicate the initialization logic used in internal/cmd/context.go so cfg.Paths.BaseConfig is populated (create the default base_config file/directory or invoke the same helper that writes defaults) and only then call filedag.New(..., filedag.WithBaseConfig(cfg.Paths.BaseConfig), ...), ensuring dagStore sees an initialized base_config path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/persis/filedag/store.go`:
- Around line 135-145: The methods useCachedLoads, useIndexedMetadata and
hasBaseConfig currently disable the fileCache/index whenever baseConfigPath
exists, causing GetMetadata/List/TagList hot paths to re-parse YAML every call;
instead ensure the cache and index remain enabled but are keyed/invalidated by
the base-config file state: record a fingerprint (mtime or checksum) for
baseConfigPath when building the cache/index and compare it in
useCachedLoads/useIndexedMetadata (or subscribe to file changes) so you only
rebuild the cache/index when the base-config file actually changed; update
functions referencing baseConfigPath/fileExists (hasBaseConfig) to check
fingerprint equality and trigger invalidation rather than blanket disabling.
---
Nitpick comments:
In `@internal/test/helper.go`:
- Around line 246-251: Before calling filedag.New to create dagStore, ensure the
default base config is created at cfg.Paths.BaseConfig exactly as done in
production startup; replicate the initialization logic used in
internal/cmd/context.go so cfg.Paths.BaseConfig is populated (create the default
base_config file/directory or invoke the same helper that writes defaults) and
only then call filedag.New(..., filedag.WithBaseConfig(cfg.Paths.BaseConfig),
...), ensuring dagStore sees an initialized base_config path.
🪄 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: 03b7273f-92ed-406d-9b8d-1c7660ecbcd1
📒 Files selected for processing (7)
internal/cmd/context.gointernal/persis/filedag/store.gointernal/persis/filedag/store_test.gointernal/service/frontend/api/v1/dags.gointernal/service/frontend/api/v1/dags_test.gointernal/test/helper.gointernal/test/scheduler.go
1 participant
Summary
type: graphis honored consistentlyRoot cause
Several validation and spec-loading paths bypassed the configured base config and loaded raw DAG YAML directly. DAGs that relied on an inherited base
type: graphwere therefore validated as the defaultchain, which rejected explicitdepends.Test plan
go test ./internal/persis/filedag -count=1go test ./internal/cmd -run 'TestRebuildDAGFromYAML_ReappliesBaseConfigContent|TestRestoreDAGFromStatus_RestoresRegistryAuthsFromBaseConfig' -count=1go test ./internal/core/spec -run 'TestLoad(_TypeInheritanceFromBaseConfig|YAMLWithOpts_TypeInheritanceInMultiDocumentYAML)' -count=1go test ./internal/service/frontend/api/v1 -count=1Closes #1979
Summary by CodeRabbit
New Features
Refactor
Tests