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

Remove usage info from log output #1242

Merged
merged 2 commits into from
Nov 13, 2023
Merged

Conversation

bjee19
Copy link
Contributor

@bjee19 bjee19 commented Nov 10, 2023

Remove usage info from log output.

Problem: When a critical error occurs at startup, usage information for the binary is dumped into the log output.

Solution: Set SilenceUsage and SilenceErrors to true in the root cobra.Command. This problem was discussed here in an older discussion on cobra's github page.

Testing: Manually tested these cases (usage is there before the change and is no longer there after the change):

  • Running docker restart kind-control-plane which displayed the error recorded in a non-functional test here.
  • Not creating the NginxGateway CRD on startup.

Please focus on (optional): Should I set SilenceUsage in createProvisionerModeCommand? Currently its just in the StaticModeCommand. Also, as described in the discussion linked above, it mentions possibly setting SilenceUsage in the root command, should I instead put that in createRootCommand?

Closes #1105

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@bjee19 bjee19 requested a review from a team as a code owner November 10, 2023 20:03
@github-actions github-actions bot added the bug Something isn't working label Nov 10, 2023
@kate-osborn
Copy link
Contributor

I think we should also set SilenceError to true so we don't print the error message twice:

without SilenceError:

{"level":"info","ts":"2023-11-10T14:24:48-06:00","msg":"Starting NGINX Gateway Fabric in static mode","version":"edge","commit":"fbc33116eba7fc8e52c55dedb073d97bf8d00995","date":"2023-11-10T20:19:02Z"}
Error: error validating POD_IP environment variable: IP address must be set
error validating POD_IP environment variable: IP address must be set

with SilenceError:

{"level":"info","ts":"2023-11-10T14:31:26-06:00","msg":"Starting NGINX Gateway Fabric in static mode","version":"edge","commit":"fbc33116eba7fc8e52c55dedb073d97bf8d00995","date":"2023-11-10T20:30:42Z"}
error validating POD_IP environment variable: IP address must be set

Should I set SilenceUsage in createProvisionerModeCommand? Currently its just in the StaticModeCommand. Also, as described in the discussion linked above, it mentions possibly setting SilenceUsage in the root command, should I instead put that in createRootCommand?

I think it makes sense in the root command so that every subcommand inherits those values.

@bjee19 bjee19 merged commit ce4d1cc into nginxinc:main Nov 13, 2023
23 checks passed
@bjee19 bjee19 deleted the bug/usage-dump branch November 20, 2023 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Errors returned at startup dump binary usage info in log output
3 participants