Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rework cmd app errors, permit non-zero exit code and custom output #1337

Merged
merged 5 commits into from
Feb 3, 2025

Conversation

jchappelow
Copy link
Member

@jchappelow jchappelow commented Feb 3, 2025

This changes how the command line apps handle errors to ensure we have non-zero exit codes to the OS/shell while also maintaining our own output format. This is important for scripting among other things.

The problem with cobra is that if we return an error from say RunE, cobra will insist on prefixing the error message with a text like Error: which is no good for reasons. We can sorta set this prefix, but it's a somewhat flawed implementation whereby we always end up with at least two spaces prefixing the output we want. This is why we have had the shared/display.PrintErr always eating the error, I believe.

We get around this issue by continuing to swallow the error, but storing the error in the command's context, and then in main() pulling it out and setting the exit code if it is not nil. (we don't actually need the error, just a bool, but I include the whole thing for debugging).

Finally, this PR also resolves related issues with startup and shutdown:

  • the buildServer function was receiving the background context (my bad) so we couldn't really cancel things like a peer dial that would have to timeout instead
  • the P2Pservice initializer used a background context
  • the bootnode loop in (*P2PService).Start always returned nil even if the context was cancelled and a peer connect returned a non-nil error. This was logically done to allow the loop to continue to other peers, but if the context was cancelled, we need to abort startup. Before this fix, it would go on to initialize other systems, eventually hitting one that cared about the context cancellation.

brennanjl
brennanjl previously approved these changes Feb 3, 2025
Copy link
Collaborator

@brennanjl brennanjl left a comment

Choose a reason for hiding this comment

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

Nice, looks like a good fix, thanks for doing this.

Will let you merge so that you can wait for your integration tests to finish

@jchappelow jchappelow merged commit 7ed1d5f into kwilteam:main Feb 3, 2025
2 checks passed
@jchappelow jchappelow deleted the printerr2 branch February 3, 2025 23:33
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.

3 participants