Skip to content

Conversation

@kurotych
Copy link
Member

@kurotych kurotych commented Mar 20, 2025

No description provided.

@kurotych kurotych changed the title Print error if verification failed Use batch_info v2 in mobile_config_cli Mar 20, 2025
Ok(()) => Some(res),
Err(_) => None,
Err(e) => {
eprintln!("Response verification failed. {e}");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
eprintln!("Response verification failed. {e}");
tracing::error!(?err, "Response verification failed");

Copy link
Contributor

Choose a reason for hiding this comment

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

do these tracing logs get output to the terminal stdout normally when operating the commands since this is in the context of an actual CLI (not just a clap construct for launching a server process) @michaeldjeffrey ? This seems like the one place where println! and eprintln! would be appropriate

@kurotych kurotych marked this pull request as ready for review March 21, 2025 12:46
Copy link
Contributor

@jeffgrunewald jeffgrunewald left a comment

Choose a reason for hiding this comment

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

Looks good to me, assuming the custom tracing and tracing crate operations will output to a cli terminal as we would expect since we're in a foreground process operating directly with the application

@kurotych kurotych merged commit e810975 into main Apr 1, 2025
27 checks passed
@kurotych kurotych deleted the mob-conf-cli-batch-v2 branch April 1, 2025 15:59
macpie pushed a commit that referenced this pull request Apr 14, 2025
* Print error if verification failed

* Use v2 requests

* Use tracing::error to print error message

* Add DeploymentInfo
macpie pushed a commit that referenced this pull request Apr 14, 2025
* Print error if verification failed

* Use v2 requests

* Use tracing::error to print error message

* Add DeploymentInfo
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.

5 participants