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

Cleanup errors caught by megacheck #85

Merged
merged 1 commit into from
May 25, 2017
Merged

Conversation

joehandzik
Copy link
Contributor

Signed-Off-By: Joe Handzik joseph.t.handzik@hpe.com

@joehandzik joehandzik requested a review from roclark May 25, 2017 18:09
Signed-Off-By: Joe Handzik <joseph.t.handzik@hpe.com>
for _, item := range jobList {
metricList = append(metricList, item)
}
metricList = append(metricList, jobList...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want (need?) to check if jobList is not nil before this line? Say there aren't any jobs in the job_stats file (happens whenever no jobs have been running for a period of time), the for loop on line 611 would have zero iterations and jobList would never be initialized. I'm not sure how the append(...) command handles nil elements. If it handles them gracefully, no issue, otherwise we might want to add a check beforehand.

Copy link
Contributor

@roclark roclark 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 outside of my comments about the append functions. Once we get those figured out I can merge!

for _, item := range statsList {
metricList = append(metricList, item)
}
metricList = append(metricList, statsList...)
Copy link
Contributor

Choose a reason for hiding this comment

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

See my other comment - we do the opposite here. Might be good to stay consistent.

Copy link
Contributor

@roclark roclark left a comment

Choose a reason for hiding this comment

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

Sorry for the double review, the second comment wasn't added to the first one for some reason.

@joehandzik
Copy link
Contributor Author

@roclark PR, now with 100% more consistency.

@roclark roclark merged commit 35b3ee0 into master May 25, 2017
@joehandzik joehandzik deleted the wip-fix-megacheck-issues branch May 25, 2017 20:09
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