sea-orm-cli errors exit with error code 0

[AngelOnFira]

Description

When using the CLI, any error returns the status code 0, which suggests that the command worked properly.

Steps to Reproduce

1. Write an incorrect migration
2. Run sea-orm-cli migrate; echo "status = $?"

Expected Behavior

sea-orm-cli migrate; echo "status = $?" should output status = 1 or some other code.

Actual Behavior

sea-orm-cli migrate; echo "status = $?" outputs status = 0, suggesting that the command worked.

Reproduces How Often

Always

Versions

│   ├── sea-orm v0.10.6
│   │   ├── sea-orm-macros v0.10.6 (proc-macro)
│   │   ├── sea-query v0.27.2
│   │   │   ├── sea-query-derive v0.2.0 (proc-macro)
│   │   ├── sea-query-binder v0.2.2
│   │   │   ├── sea-query v0.27.2 (*)
│   │   ├── sea-strum v0.23.0
│   │   │   └── sea-strum_macros v0.23.0 (proc-macro)
├── sea-orm v0.10.6 (*)

Additional Information

This is likely quite easy to fix, it just requires exiting with error code 1 on any error 🙂

My particular use case is I have a script to run migrations in development that looks like the following:

dropdb -U postgres -h localhost -p 5432 -w postgres
createdb -U postgres -h localhost -p 5432 -w postgres
sea-orm-cli migrate \
    && sea-orm-cli generate entity \
        -o entity/src/entities \
        --expanded-format \
        --with-serde both

In this case, I want the script to stop running if the migration failed, since it's easier to debug without the rest of the data from generating entities. The && will only run the second command if the first one exits with a return value of 0.

[Diwakar-Gupta]

I did reproduce this issue and would like to work on it.
@tyt2y3 @billy1624
I saw no member has commented on this can you please check it once?

Implementation

let status = Command::new("mkdir")
                     .arg("projects")
                     .status()
                     .expect("failed to execute mkdir");

we can use this status.code() and terminate out program with same status code.
Affected files from this implementation will be sea-orm-cli/src/commands/migrate.rs.

[billy1624]

Hey @AngelOnFira, thanks for the report. After some digging I found that we didn't check the status code of the executed command.

To handle it properly, we should:

// Run migrator CLI on user's behalf
println!("Running `cargo {}`", args.join(" "));
let exit_status = Command::new("cargo").args(args).status()?; // Get the status code
if !exit_status.success() { // Propagate the error if any
    return Err("Fail to run migration".into());
}

Source code location:

sea-orm/sea-orm-cli/src/commands/migrate.rs

Lines 67 to 69 in ca20554

	// Run migrator CLI on user's behalf
	println!("Running `cargo {}`", args.join(" "));
	Command::new("cargo").args(args).spawn()?.wait()?;

I simply thrown an error and it will be handled by handle_error function at

sea-orm/sea-orm-cli/src/bin/main.rs

Lines 18 to 30 in ca20554

	Commands::Migrate {
	migration_dir,
	database_schema,
	database_url,
	command,
	} => run_migrate_command(
	command,
	&migration_dir,
	database_schema,
	database_url,
	verbose,
	)
	.unwrap_or_else(handle_error),

[billy1624]

Hey @Diwakar-Gupta, thanks for the initiative! Any opinions?
Maybe we should double check all places where we have used std::process::Command?

[Diwakar-Gupta]

@billy1624
I did checked the suggested code and it worked.

Apart from sea-orm-cli/src/commands/migrate.rs we also need to handle same in sea-orm-cli/src/commands/generate.rs file.
I will raise a PR with this changes soon.

Thanks for help.

[billy1624]

Thanks again for contributing! :D

[AngelOnFira]

Thanks for the fix @Diwakar-Gupta!