fix(seed): leverage a local Fern CLI whenever you're using one in a seed script #4853
Conversation
WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on the The Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
- packages/seed/src/cli.ts (1 hunks)
- packages/seed/src/commands/run/runWithCustomFixture.ts (1 hunks)
- packages/seed/src/commands/test/ScriptRunner.ts (3 hunks)
- seed/python-sdk/seed.yml (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
seed/python-sdk/seed.yml (2)
Line range hint
1-265: Summary of changes: Improved test execution processThe main changes in this file are focused on improving the test execution process for both Pydantic V1 and V2. The introduction of the
FERN_NO_VERSION_REDIRECTIONenvironment variable and the use of a Node.js script to run tests suggest a more standardized approach to testing across different Pydantic versions.These changes appear to be part of a larger effort to enhance the testing infrastructure, potentially providing more consistent results across different environments. The rest of the configuration file remains unchanged, maintaining the existing structure for build scripts, publish settings, and fixture configurations.
238-238: LGTM! Verify the new test execution method.The changes to the test execution command look good. The introduction of
FERN_NO_VERSION_REDIRECTION=trueand the use of a Node.js script to run tests appear to be intentional modifications to standardize the testing process.To ensure the new test execution method works as expected, please run the following verification steps:
Test with Pydantic V2:
Test with Pydantic V1:
Verify that both commands execute successfully and produce the expected test results.
Also applies to: 249-249
packages/seed/src/cli.ts (1)
118-122: LGTM: Improved context management for ScriptRunnerThe addition of a dedicated task context for the ScriptRunner enhances the granularity of context management within the testing framework. This change aligns well with the PR objective of leveraging a local Fern CLI in seed scripts.
packages/seed/src/commands/test/ScriptRunner.ts (2)
166-167: Ensure the CLI volume bind path is validThe
cliVolumeBindvariable uses the path fromabsoluteFilePathToFernCli. Verify that this path exists and is accessible to Docker. In environments with different directory structures, this may cause issues. Consider adding checks or configurations to validate the path before usage.
41-47: Ensure all constructor calls include the newcontextparameterThe
ScriptRunnerconstructor now requires an additionalcontext: TaskContextparameter. Verify that all instances whereScriptRunneris instantiated have been updated to pass this new parameter to prevent runtime errors.Run the following script to identify constructor calls that may need updating:
| @@ -49,7 +49,7 @@ export async function runWithCustomFixture({ | |||
| taskContextFactory, | |||
| skipScripts: true, | |||
| keepDocker: true, | |||
| scriptRunner: new ScriptRunner(workspace, false) | |||
| scriptRunner: new ScriptRunner(workspace, false, taskContext) | |||
There was a problem hiding this comment.
💡 Codebase verification
Issue Detected: Missing taskContext in ScriptRunner Instantiation
The ScriptRunner instantiation in packages/seed/src/cli.ts does not include the taskContext parameter:
const scriptRunner = new ScriptRunner(Please update this instantiation to include taskContext to ensure consistency and alignment with the PR objectives.
🔗 Analysis chain
LGTM. Please verify if this change fully addresses the PR objective.
The addition of taskContext to the ScriptRunner constructor looks good and doesn't introduce any obvious issues. However, I have a couple of suggestions:
-
Could you verify if this change alone is sufficient to "leverage a local Fern CLI whenever you're using one in a seed script" as mentioned in the PR title? If not, are there additional changes needed?
-
Consider adding a brief comment explaining the purpose of passing
taskContextto theScriptRunner. This would help future developers understand the intent behind this change.
To ensure this change is consistent across the codebase, please run the following script:
This will help identify any other locations where ScriptRunner is instantiated and might need a similar update.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other instances of ScriptRunner instantiation
# that might need to be updated with the taskContext parameter.
# Search for ScriptRunner instantiations
rg --type typescript "new ScriptRunner\(" -g '!**/runWithCustomFixture.ts'
Length of output: 111
Script:
#!/bin/bash
# Description: Check for other instances of ScriptRunner instantiation
# that might need to be updated with the taskContext parameter.
# Search for ScriptRunner instantiations in TypeScript files, excluding runWithCustomFixture.ts
rg "new ScriptRunner\(" -g "*.ts" -g '!**/runWithCustomFixture.ts'
Length of output: 148
Script:
#!/bin/bash
# Description: Verify if the ScriptRunner instantiation in packages/seed/src/cli.ts includes the taskContext parameter.
# Search for the ScriptRunner instantiation in cli.ts
script_instantiation=$(rg "new ScriptRunner\(" packages/seed/src/cli.ts)
# Check if 'taskContext' is present in the instantiation
echo "$script_instantiation" | grep -q "taskContext"
if [ $? -eq 0 ]; then
echo "taskContext parameter is present in ScriptRunner instantiation."
else
echo "taskContext parameter is MISSING in ScriptRunner instantiation."
fi
Length of output: 372
| private async startContainers(): Promise<void> { | ||
| private async buildFernCli(context: TaskContext): Promise<AbsoluteFilePath> { | ||
| await loggingExeca(context.logger, "pnpm", ["fern:build"]); | ||
| return join(AbsoluteFilePath.of(__dirname), RelativeFilePath.of("../../../packages/cli/cli/dist/prod")); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify the correctness of the path resolution
The relative path used in buildFernCli may not resolve correctly in all environments. Ensure that the constructed path to the built CLI (../../../packages/cli/cli/dist/prod) is accurate and consider making it configurable or using environment variables to enhance portability.
| private async buildFernCli(context: TaskContext): Promise<AbsoluteFilePath> { | ||
| await loggingExeca(context.logger, "pnpm", ["fern:build"]); | ||
| return join(AbsoluteFilePath.of(__dirname), RelativeFilePath.of("../../../packages/cli/cli/dist/prod")); | ||
| } |
There was a problem hiding this comment.
Handle possible failures in buildFernCli method
The pnpm fern:build command may fail, and currently, any errors thrown by loggingExeca are not being caught. To ensure robustness, consider adding error handling to catch and handle exceptions, providing clear error messages or taking remedial actions as needed.
Apply this diff to handle potential errors:
private async buildFernCli(context: TaskContext): Promise<AbsoluteFilePath> {
- await loggingExeca(context.logger, "pnpm", ["fern:build"]);
+ const buildCommand = await loggingExeca(context.logger, "pnpm", ["fern:build"], { reject: false });
+ if (buildCommand.failed) {
+ context.logger.error("Failed to build Fern CLI. See output below:");
+ context.logger.error(buildCommand.stdout);
+ context.logger.error(buildCommand.stderr);
+ throw new Error("Fern CLI build failed");
+ }
return join(
AbsoluteFilePath.of(__dirname),
RelativeFilePath.of("../../../packages/cli/cli/dist/prod")
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private async buildFernCli(context: TaskContext): Promise<AbsoluteFilePath> { | |
| await loggingExeca(context.logger, "pnpm", ["fern:build"]); | |
| return join(AbsoluteFilePath.of(__dirname), RelativeFilePath.of("../../../packages/cli/cli/dist/prod")); | |
| } | |
| private async buildFernCli(context: TaskContext): Promise<AbsoluteFilePath> { | |
| const buildCommand = await loggingExeca(context.logger, "pnpm", ["fern:build"], { reject: false }); | |
| if (buildCommand.failed) { | |
| context.logger.error("Failed to build Fern CLI. See output below:"); | |
| context.logger.error(buildCommand.stdout); | |
| context.logger.error(buildCommand.stderr); | |
| throw new Error("Fern CLI build failed"); | |
| } | |
| return join( | |
| AbsoluteFilePath.of(__dirname), | |
| RelativeFilePath.of("../../../packages/cli/cli/dist/prod") | |
| ); | |
| } |
There was a problem hiding this comment.
@armandobelardo great suggestion by the AI imo
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/seed/src/commands/test/ScriptRunner.ts (3 hunks)
- seed/python-sdk/seed.yml (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- seed/python-sdk/seed.yml
🧰 Additional context used
🔇 Additional comments (2)
packages/seed/src/commands/test/ScriptRunner.ts (2)
1-1: LGTM: Constructor and import changes are consistent with new functionality.The new imports and the updated constructor signature align well with the changes made to the class. The addition of the
contextparameter to both the constructor and thestartContainersmethod call is consistent and necessary for the new functionality.Also applies to: 41-47
Line range hint
1-178: Addressing past review commentsI've noticed that there were past review comments about error handling and path resolution. Our current review has addressed these concerns:
- We've suggested error handling improvements for both
buildFernCliandstartContainersmethods.- We've recommended making the CLI path more configurable, which partially addresses the path resolution concern.
These suggestions, if implemented, should resolve the issues raised in the past comments.
| private async startContainers(context: TaskContext): Promise<void> { | ||
| const absoluteFilePathToFernCli = await this.buildFernCli(context); | ||
| const cliVolumeBind = `${absoluteFilePathToFernCli}:/fern`; | ||
| // Start running a docker container for each script instance | ||
| for (const script of this.workspace.workspaceConfig.scripts ?? []) { | ||
| const startSeedCommand = await loggingExeca(undefined, "docker", ["run", "-dit", script.docker, "/bin/sh"]); | ||
| const startSeedCommand = await loggingExeca(undefined, "docker", [ | ||
| "run", | ||
| "-dit", | ||
| "-v", | ||
| cliVolumeBind, | ||
| script.docker, | ||
| "/bin/sh" | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for Docker container startup.
The changes to use the local Fern CLI are good, but we should add error handling for the Docker container startup to improve robustness.
Consider adding error handling like this:
private async startContainers(context: TaskContext): Promise<void> {
const absoluteFilePathToFernCli = await this.buildFernCli(context);
const cliVolumeBind = `${absoluteFilePathToFernCli}:/fern`;
for (const script of this.workspace.workspaceConfig.scripts ?? []) {
const startSeedCommand = await loggingExeca(undefined, "docker", [
"run",
"-dit",
"-v",
cliVolumeBind,
script.docker,
"/bin/sh"
], { reject: false });
if (startSeedCommand.failed) {
context.logger.error("Failed to start Docker container. See output below:");
context.logger.error(startSeedCommand.stdout);
context.logger.error(startSeedCommand.stderr);
throw new Error("Docker container startup failed");
}
const containerId = startSeedCommand.stdout;
this.scripts.push({ ...script, containerId });
}
}
dsinghvi
force-pushed
the
ab/local-cli
branch
from
1144581 to
2759f86
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
- packages/seed/src/cli.ts (1 hunks)
- packages/seed/src/commands/run/runWithCustomFixture.ts (1 hunks)
- packages/seed/src/commands/test/ScriptRunner.ts (3 hunks)
- seed/python-sdk/seed.yml (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/seed/src/cli.ts
- packages/seed/src/commands/run/runWithCustomFixture.ts
- seed/python-sdk/seed.yml
🧰 Additional context used
🔇 Additional comments (3)
packages/seed/src/commands/test/ScriptRunner.ts (3)
1-1: LGTM: New imports added correctly.The new imports (
AbsoluteFilePath,join, andRelativeFilePath) from "@fern-api/fs-utils" are correctly added and will be used in the newbuildFernClimethod.
41-47: LGTM: Constructor updated correctly.The constructor has been properly updated to include the new
contextparameter of typeTaskContext. This change is consistent with the modifications in thestartContainersmethod, and thecontextis correctly passed tostartContainers.
166-178:⚠️ Potential issueImprove error handling in
startContainersand verify path resolution.The changes to use the local Fern CLI are good, but we should add error handling for the Docker container startup to improve robustness. Also, ensure that the path resolution for the CLI is correct across different environments.
- Add error handling:
private async startContainers(context: TaskContext): Promise<void> { const absoluteFilePathToFernCli = await this.buildFernCli(context); const cliVolumeBind = `${absoluteFilePathToFernCli}:/fern`; for (const script of this.workspace.workspaceConfig.scripts ?? []) { const startSeedCommand = await loggingExeca(undefined, "docker", [ "run", "-dit", "-v", cliVolumeBind, script.docker, "/bin/sh" ], { reject: false }); if (startSeedCommand.failed) { context.logger.error("Failed to start Docker container. See output below:"); context.logger.error(startSeedCommand.stdout); context.logger.error(startSeedCommand.stderr); throw new Error("Docker container startup failed"); } const containerId = startSeedCommand.stdout; this.scripts.push({ ...script, containerId }); } }
- Verify the correctness of the CLI path resolution:
This script verifies the existence of the CLI path and executable, ensuring that the path resolution is correct across different environments.
| private async buildFernCli(context: TaskContext): Promise<AbsoluteFilePath> { | ||
| const rootDir = join(AbsoluteFilePath.of(__dirname), RelativeFilePath.of("../../..")); | ||
| await loggingExeca(context.logger, "pnpm", ["fern:build"], { cwd: rootDir }); | ||
| return join(rootDir, RelativeFilePath.of("packages/cli/cli/dist/prod")); | ||
| } |
There was a problem hiding this comment.
Add error handling to buildFernCli method.
While the method correctly builds the Fern CLI, it lacks error handling for potential build failures. Consider adding error handling to improve robustness.
Here's a suggested implementation with error handling:
private async buildFernCli(context: TaskContext): Promise<AbsoluteFilePath> {
const rootDir = join(AbsoluteFilePath.of(__dirname), RelativeFilePath.of("../../.."));
const result = await loggingExeca(context.logger, "pnpm", ["fern:build"], { cwd: rootDir, reject: false });
if (result.failed) {
context.logger.error("Failed to build Fern CLI. See output below:");
context.logger.error(result.stdout);
context.logger.error(result.stderr);
throw new Error("Fern CLI build failed");
}
return join(rootDir, RelativeFilePath.of("packages/cli/cli/dist/prod"));
}
dsinghvi
force-pushed
the
ab/local-cli
branch
from
8c5accb to
712ebd5
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Documentation