Dry run before publishing - #85
Conversation
Ravion Module Publish PlanDry run only. No Ravion API mutations were made. No publish changes are required. All versions already exist with identical config. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (dryRun && options.validateRemote) { | ||
| if (!remoteDefinition) { | ||
| options.logger?.( | ||
| `Skipping remote validation for ${definition.type}@${definition.version}; the module definition does not exist remotely yet.`, | ||
| ); |
There was a problem hiding this comment.
When a new module has config accepted locally but rejected by the Ravion API, both dry runs skip remote validation because the definition does not exist yet. The later --apply run creates and globally publishes the definition before version creation fails, leaving a published definition with no version and failing the main-branch publish workflow.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tools/ravion-modules/src/publish.ts
Line: 238-242
Comment:
**New definitions bypass validation**
When a new module has config accepted locally but rejected by the Ravion API, both dry runs skip remote validation because the definition does not exist yet. The later `--apply` run creates and globally publishes the definition before version creation fails, leaving a published definition with no version and failing the main-branch publish workflow.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| if (result.validationErrors && result.validationErrors.length > 0) { | ||
| lines.push( | ||
| "### 🚨 Remote Validation Failures 🚨", | ||
| "", | ||
| "The Ravion API rejected these module version configs during dry-run validation. Fix the module definition config before merging.", | ||
| "", | ||
| "| Module | Release Version | Error |", | ||
| "| --- | --- | --- |", | ||
| ); | ||
| for (const error of result.validationErrors) { | ||
| lines.push( | ||
| `| \`${escapeMarkdownTableCell(error.type)}\` | \`${escapeMarkdownTableCell(error.version)}\` | ${escapeMarkdownTableCell(error.message)} |`, | ||
| ); | ||
| } | ||
| lines.push(""); | ||
| return lines.join("\n"); |
There was a problem hiding this comment.
When one module fails remote validation, the result still contains planned changes for every processed definition, but this early return renders only the failure table. The workflow uses this Markdown directly for the PR comment, so reviewers cannot see the other valid releases or their diffs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tools/ravion-modules/src/publish.ts
Line: 340-355
Comment:
**Validation errors hide plan items**
When one module fails remote validation, the result still contains planned changes for every processed definition, but this early return renders only the failure table. The workflow uses this Markdown directly for the PR comment, so reviewers cannot see the other valid releases or their diffs.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| it("validates create-version items remotely during dry run when requested", async () => { | ||
| const client = new MockRavionClient({ | ||
| definitions: [{ id: "vpc", type: "ravion-aws-vpc", name: "AWS VPC", description: "AWS VPC and subnets." }], | ||
| }); | ||
|
|
||
| const result = await publishDefinitions([createCompiledDefinition()], client, { dryRun: true, validateRemote: true }); | ||
|
|
||
| assert.deepEqual(result.items.map(({ action, dryRun }) => ({ action, dryRun })), [{ action: "create-version", dryRun: true }]); | ||
| assert.deepEqual(client.validatedVersions, [ | ||
| { moduleDefinitionId: "vpc", version: "1.2.3", description: "Add subnet options.", config: { inputs: [{ id: "name", type: "string", label: "Name" }] } }, | ||
| ]); | ||
| assert.equal(client.createdVersions.length, 0); | ||
| }); |
There was a problem hiding this comment.
This test uses MockRavionClient, so it does not exercise the changed HTTP request envelope or the strict 202 response check in HttpRavionModuleApiClient.validateModuleVersion. Add an HTTP-client test for the dry-run request and success status so a mismatch with the API contract does not make every validation workflow fail.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tools/ravion-modules/test/publish.test.ts
Line: 56-68
Comment:
**HTTP validation path remains untested**
This test uses `MockRavionClient`, so it does not exercise the changed HTTP request envelope or the strict `202` response check in `HttpRavionModuleApiClient.validateModuleVersion`. Add an HTTP-client test for the dry-run request and success status so a mismatch with the API contract does not make every validation workflow fail.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Greptile Summary
This PR adds optional Ravion API validation to publish dry runs and surfaces validation failures in the generated PR plan.
--validate-remoteCLI option and enables it in PR and main-branch dry-run workflow steps.Confidence Score: 4/5
The PR needs fixes before merging because new definitions bypass remote validation and mixed validation failures hide part of the publish plan.
A rejected first version can leave a newly created definition globally published without a version, while the Markdown error path omits otherwise valid changes retained in the generated plan; the HTTP-specific validation contract also lacks direct test coverage.
Files Needing Attention: tools/ravion-modules/src/publish.ts, .github/workflows/module-definitions.yml, and tools/ravion-modules/test/publish.test.ts
Important Files Changed
--validate-remoteoption while preserving structured error output.Sequence Diagram
sequenceDiagram participant CI as GitHub Actions participant CLI as ravion-modules CLI participant API as Ravion API CI->>CLI: publish --validate-remote CLI->>API: Load remote inventory alt Definition already exists CLI->>API: "POST module version with dryRun=true" API-->>CLI: Validation result else New definition CLI-->>CI: Skip remote validation end CI->>CLI: publish --apply CLI->>API: Create definition CLI->>API: Mark definition globally published CLI->>API: Create version API-->>CLI: Success or config rejectionPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "Merge branch 'main' into ma/publish-dry-..." | Re-trigger Greptile