-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fix for panic when submitting non-existent version for job history CLI command #4577
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should be doing a nil check on the job from the caller as well? Seems like a caller error if it's wanting to format a nil job.
I'm ok with this getting merged though.
if job == nil { | ||
return fmt.Errorf("Error printing job history for non-existing job or job version") | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add a test case for this in job_history_test.go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are not any success case tests for job_history
or similar commands such as job_inspect
or job_revert
- I tried a couple different test setups but wasn't able to get a successful test passing for this. I'm going to merge as is but we can look at what a success test case could look like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be nice to verify with a unit test, otherwise LGTM
I know this is merged already but was wondering if it would have been better to put the nil check on line 173? That is the first place you know job is nil. That would avoid possibly passing a nil job to the Format call on line 175. |
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Fixes a panic where running
nomad job history -full -version=500 example
would result in:Fixes #4542