Skip to content

Conversation

@Arrowana
Copy link
Contributor

@Arrowana Arrowana commented Apr 23, 2025

Problem

The API of Command looks misunderstood, it does not err on exit code not being a success but only if invocation of the command itself fails (command not found...).

This leads to messed up CLI output, one has to read what happened above to deduce the actual real outcome of the command since it continues on failure silently.

A typical case is compilation failure then a program hash gets printed.

I think this can be more serious in some cases where a sequence of actions can lead to garbage unrelated hash in silence.

Solution

Unfortunately there is no direct api it seems to err on status code. Use ensure! macro to validate the success

I didn't use the status code on anything that looks like a cleanup to avoid a failure and further cleanup

@ngundotra
Copy link
Collaborator

ngundotra commented Apr 29, 2025

Reviewing this today. Do you have a command for me to check that this PR works?

@ngundotra ngundotra linked an issue Apr 29, 2025 that may be closed by this pull request
Copy link

@jasonslagter jasonslagter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need

@jasonslagter
Copy link

Problem

The API of Command looks misunderstood, it does not err on exit code not being a success but only if invocation of the command itself fails (command not found...).

This leads to messed up CLI output, one has to read what happened above to deduce the actual real outcome of the command since it continues on failure silently.

A typical case is compilation failure then a program hash gets printed.

I think this can be more serious in some cases where a sequence of actions can lead to garbage unrelated hash in silence.

Solution

Unfortunately there is no direct api it seems to err on status code. Use ensure! macro to validate the success

I didn't use the status code on anything that looks like a cleanup to avoid a failure and further cleanup

Copy link
Collaborator

@swaroop-osec swaroop-osec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Please bump the crate version in Cargo.toml to "0.4.4"
  • It would be great to run cargo clippy and apply suggestions to keep the codebase clean and idiomatic.
  • Consider adding more descriptive error messages to the ensure! macros. This will improve debugging and make logs clear.

@swaroop-osec
Copy link
Collaborator

swaroop-osec commented Apr 30, 2025

@Arrowana This works well as intended, verified locally. Requested a few minor changes to polish it up.
CC: @ngundotra

@swaroop-osec
Copy link
Collaborator

lgtm

@ngundotra ngundotra merged commit 86e1788 into Ellipsis-Labs:master May 1, 2025
8 checks passed
use anyhow::anyhow;
use anyhow::{anyhow, ensure};
use api::{
get_last_deployed_slot, get_remote_job, get_remote_status, send_job_with_uploader_to_remote,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Build failures are not detected

4 participants