Conversation
… tests Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates wfctl docs generate to correctly handle multi-workflow ApplicationConfig inputs (where an application.workflows[] list references multiple YAML files), ensuring documentation is generated from the fully loaded configuration rather than silently producing empty output.
Changes:
- Detect
ApplicationConfigby peeking raw YAML bytes and, when present, load + merge referenced workflow files into a singleWorkflowConfig. - Use
application.nameas the default docs title (unless-titleis provided). - Render an “Application Workflows” table in
README.mdlisting each referenced workflow file, and add a suite of tests covering generation and content.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/wfctl/docs.go | Adds ApplicationConfig detection/merge path, uses application name for title, and emits an Application Workflows table in README. |
| cmd/wfctl/docs_test.go | Adds tests validating docs generation and content for an app config referencing multiple workflow files, including title override behavior. |
cmd/wfctl/docs.go
Outdated
| var cfg *config.WorkflowConfig | ||
| var appInfo *config.ApplicationInfo | ||
| if config.IsApplicationConfig(rawData) { | ||
| appCfg, appErr := config.LoadApplicationConfig(configFile) | ||
| if appErr != nil { | ||
| return fmt.Errorf("failed to load application config: %w", appErr) | ||
| } | ||
| merged, mergeErr := config.MergeApplicationConfig(appCfg) | ||
| if mergeErr != nil { | ||
| return fmt.Errorf("failed to merge application config: %w", mergeErr) | ||
| } | ||
| cfg = merged | ||
| appInfo = &appCfg.Application | ||
| } else { |
|
@copilot apply changes based on the comments in this thread |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
…extract test helper Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Done in commit 46b4deb. Both review points addressed:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes wfctl docs generate so it can correctly generate documentation from an ApplicationConfig (with application.workflows[]), by loading and merging all referenced workflow files into a single WorkflowConfig.
Changes:
- Detect
ApplicationConfiginputs and merge embedded workflow configs before generating docs. - Enhance README generation to use
application.nameas the default title and list embedded workflow sources. - Add deep-merge behavior for specific list-bearing workflow sections when multiple workflow files contribute to the same workflow key, plus new tests covering these scenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
cmd/wfctl/docs.go |
Detects ApplicationConfig, merges referenced workflow files, uses app name as default title, and renders an “Application Workflows” table in README. |
config/config.go |
Adds mergeWorkflowSection and uses it during MergeApplicationConfig to append specific list fields when workflow keys collide. |
cmd/wfctl/docs_test.go |
Adds ApplicationConfig fixtures/tests validating merged output and duplicate-workflow-key deep-merge behavior. |
|
@copilot apply changes based on the comments in this thread |
… test assertions Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Done in commit 8532ce3. Both issues addressed:
|
There was a problem hiding this comment.
Pull request overview
Updates wfctl docs generate to properly handle ApplicationConfig (multi-file) inputs by loading and merging all referenced workflow files, and extends generated README content to show embedded workflow sources.
Changes:
- Detect ApplicationConfig inputs and generate docs from a merged
WorkflowConfigspanning all referenced files. - Deep-merge specific list-bearing workflow subsections (
routes,subscriptions,producers,definitions) when multiple embedded workflow files share the same top-level workflow key. - Add/expand test coverage for ApplicationConfig doc generation and duplicate workflow-key merging behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
cmd/wfctl/docs.go |
Detect ApplicationConfig, merge embedded workflow files, use application.name as title fallback, and render an “Application Workflows” table in README. |
config/config.go |
Add mergeWorkflowSection and use it to append list-bearing fields when merging duplicate workflow keys across files. |
cmd/wfctl/docs_test.go |
Add ApplicationConfig fixtures and tests validating merged documentation output and title override behavior. |
config/merge_test.go |
Add focused unit tests for mergeWorkflowSection list-merge edge cases (nil/non-slice dst, append). |
| if existing, exists := combined.Workflows[k]; exists { | ||
| dstMap, dstOk := existing.(map[string]any) | ||
| srcMap, srcOk := v.(map[string]any) | ||
| if dstOk && srcOk { | ||
| mergeWorkflowSection(dstMap, srcMap) | ||
| continue | ||
| } | ||
| // Cannot deep-merge (not both maps) — first definition wins. | ||
| continue |
|
@copilot apply changes based on the comments in this thread When generating documentation, the docs should reflect how the engine sees/interprets the yaml. So if the engine doesn't merge pipelines that are named the same, neither should the docs. We should ensure some consistency. Add more tests with more complex scenarios and validate the outcomes are as expected. Maybe as part of docs generation, start with config validation before generating docs so if there's any unsupported scenario, we capture it early. |
…in docs; add validation + complex tests Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Done in commit 86efa1a. Here's a summary of what changed:
|
wfctl docs generatesilently produced empty/useless documentation when given an ApplicationConfig (application.workflows[]format), because it usedconfig.LoadFromFilewhich only handles plainWorkflowConfig— ignoring all referenced sub-files entirely.Changes
cmd/wfctl/docs.goconfig.NewFileSource(file).Load(ctx)— the same code path the core engine uses — to load and merge the configuration, ensuring docs always reflect exactly what the engine would seeapplication.nameand the workflow list for the README title and tableapplication.nameas the doc title (falls back to filename-derived title;-titleflag still overrides)appInfo *config.ApplicationInfotodocsGenerator; render an Application Workflows table inREADME.mdlisting each embedded file and its nameschema.ValidateConfig(with lenient options: skip type checks, allow empty modules/no entry points) before generating docs so structural issues are surfaced earlyconfig/config.gomergeWorkflowSectionhelper used byMergeApplicationConfigto deep-merge list-bearing fields (routes,subscriptions,producers,definitions) when multiple embedded workflow files define the same top-level workflow key. Nil/non-slice destination values are treated as empty so later files' list items are never silently dropped. Scalar/map keys continue to use first-definition-wins semantics.MergeApplicationConfig: when a workflow key already exists with anilvalue (e.g.http:with no body in YAML), the incoming file's valid value is now used instead of being silently dropped by the unconditionalcontinuecmd/wfctl/docs_test.gobuildAppConfigFixturehelper to eliminate repeated temp-dir setup across ApplicationConfig test casesdocsFullConfiganddocsAPIWorkflowConfigfixtures: addedsecret: "$JWT_SECRET"toauth.jwtmodules (required field now caught by the new pre-generation validation)chimera-platformapp with two embedded workflow files (api/api.yaml,jobs/application.yaml), verifying:.mdfiles are generatedmodules.mdincludes modules from all sub-filesworkflows.mdcovers HTTP routes, messaging topics, and state machine diagrams from all sub-filespipelines.mdincludes pipelines (and compensation steps) from all sub-files-titleflag correctly overrides the application nameTestDocsApplicationConfigDuplicateWorkflowKeyverifying that routes from two workflow files sharing the sameworkflows.httpkey are both present (deep merge)TestDocsApplicationConfigDuplicateModuleName— duplicate module name across files → error (consistent with engine)TestDocsApplicationConfigDuplicatePipelineName— duplicate pipeline name across files → error (consistent with engine)TestDocsApplicationConfigNullWorkflowSection— null workflow section in first file → second file's content is usedTestDocsApplicationConfigThreeFiles— 3-file app (api + events + jobs) with full cross-file content assertionsconfig/merge_test.gomergeWorkflowSectionedge cases: nil destination list, non-slice destination replacement, and normal appendTestMergeApplicationConfig_NilWorkflowKeyReplacedByLaterFile— null workflow key in first file is replaced by second file's valueTestMergeApplicationConfig_DuplicateModuleNameReturnsErrorandTestMergeApplicationConfig_DuplicatePipelineNameReturnsError— conflict detection unit testsExample config that now works:
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.