feat(cli,sdk): add --fs option for filesystem selection#178
Open
feat(cli,sdk): add --fs option for filesystem selection#178
Conversation
Contributor
📋 Check Results✨ JS SDK - Code FormattingShow format check results🔍 JS SDK - TypeScript Type CheckShow type check output🧪 JS SDK - Test ResultsShow test output |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
635
to
644
| composeFile.pre_launch_script = preLaunchScriptContent; | ||
| } | ||
|
|
||
| if (options.fs) { | ||
| composeFile.storage_fs = options.fs; | ||
| } | ||
|
|
||
| const payload: Record<string, unknown> = { | ||
| name: name, | ||
| compose_file: composeFile, |
There was a problem hiding this comment.
Bug: When updating a CVM via phala deploy --cvm-id, the --fs option is silently ignored because updateCvm doesn't update the app_compose.storage_fs field.
Severity: MEDIUM
Suggested Fix
In the updateCvm function, update the app_compose object with the filesystem option before sending the update request. Add the following code: if (validatedOptions.fs) { app_compose.storage_fs = validatedOptions.fs; }.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: cli/src/commands/deploy/handler.ts#L635-L644
Potential issue: The `updateCvm` function, used for updating existing CVMs, does not
propagate the `fs` option to the `app_compose` object before sending the update request.
While new deployments correctly handle this option by setting `composeFile.storage_fs`,
the update path is missing this logic. As a result, if a user attempts to change the
filesystem on an existing CVM using the `--fs` flag (e.g., `phala deploy --cvm-id <id>
--fs ext4`), the flag is silently ignored, and the filesystem remains unchanged,
contrary to user expectation.
Did we get this right? 👍 / 👎 to inform future reviews.
…tion Allow users to specify ext4 or zfs filesystem when provisioning a CVM via `phala deploy --fs ext4`. The storage_fs field is set in the app-compose (compose_file) payload, which is the source of truth for filesystem selection in dstack. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Leechael
force-pushed
the
feat/deploy-fs-option
branch
from
0cfdfd1 to
15c3940
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--fsCLI flag tophala deployacceptingext4orzfs(default: zfs)storage_fsto thecompose_fileobject inProvisionCvmRequestSchemaandLooseAppComposeSchemaFiles changed
js/src/types/app_compose.tsstorage_fstoLooseAppComposeSchemajs/src/actions/cvms/provision_cvm.tsstorage_fsto provision requestcompose_fileschemacli/src/commands/deploy/command.ts--fsoption definition and schema fieldcli/src/commands/deploy/handler.tsfsintocomposeFile.storage_fsinbuildProvisionPayload()cli/src/commands/deploy/handler.test.tsTest plan
bun test cli/src/commands/deploy/handler.test.ts— 37 tests passphala deploy --helpshows--fsoption--fs ext4confirmedcompose_file.storage_fs: "ext4"in CVM info response🤖 Generated with Claude Code