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

[VC-35738] Remove os.Exit from the logs module #611

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

wallrj
Copy link
Member

@wallrj wallrj commented Nov 7, 2024

Instead of calling os.exit directly after parsing and finding an error in the logging flags, the error is now returned and handled the same way as errors that come from the Run function....they are logged using structured logging and the logs are flushed before the process exits.

I've adjusted the logs tests accordingly and modified them so that the log flag parsing and error handling in the tests matches the behaviour of the actual command.

Before:

$ ./preflight agent --logging-format=foo
Error in logging configuration: format: Invalid value: "foo": Unsupported log format

$ echo $?
2

After:

$ ./preflight agent --logging-format=foo
E1107 13:41:31.063915  281828 root.go:51] "Exiting due to error" err="Error in logging configuration: format: Invalid value: \"foo\": Unsupported log format" exit-code=1

$ echo $?
1

PersistentPreRun: func(cmd *cobra.Command, args []string) {
logs.Initialize()
PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
return logs.Initialize()
},
Copy link
Member Author

Choose a reason for hiding this comment

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

Returning the error here means that the error can be logged in the same format as all the other errors that are returned by the command.

exitCode := 1
klog.ErrorS(err, "Exiting due to error", "exit-code", exitCode)
klog.FlushAndExit(time.Second, exitCode)
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

These changes are intended to make the logs test fixture parse the flags and behave the same way as Cobra.

  • --help / -h results in stdout and exit code 0
  • Unrecognized flags result in an error which is logged to stderr with exit code 1

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the extra context. I think it would be worth adding a comment above to keep that in mind in the future. I'm already imagining myself in a year: why do we do this weird dance with the exit codes in the test code?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll merge this now and add the comment in one of the followup PRS.

@wallrj wallrj requested a review from maelvls November 7, 2024 13:46
@wallrj wallrj mentioned this pull request Nov 7, 2024
12 tasks
@wallrj wallrj changed the base branch from VC-35738/feature to VC-35738/move-one-shot-test November 7, 2024 15:40
@wallrj wallrj force-pushed the VC-35738/more-realistic-logs-tests branch from 3d4876b to a8f16c7 Compare November 7, 2024 15:44
Base automatically changed from VC-35738/move-one-shot-test to VC-35738/feature November 12, 2024 06:55
…e realistic

Signed-off-by: Richard Wall <richard.wall@venafi.com>
@wallrj wallrj force-pushed the VC-35738/more-realistic-logs-tests branch from a8f16c7 to c3b5306 Compare November 12, 2024 07:01
@wallrj wallrj changed the title [VC-35738] Remove os.Exit from the logs module and make the logs test output more realistic [VC-35738] Remove os.Exit from the logs module Nov 12, 2024
Copy link
Member

@maelvls maelvls left a comment

Choose a reason for hiding this comment

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

This sounds like a good change. I haven't tested the change manually nor have I run the automated tests, I trust you.

@wallrj wallrj merged commit 39e6c1f into VC-35738/feature Nov 12, 2024
2 checks passed
@wallrj wallrj deleted the VC-35738/more-realistic-logs-tests branch November 12, 2024 14:11
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.

2 participants