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.

Sign up for GitHub

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

Jump to bottom

Fix runner report and json newline #2601

Merged
merged 4 commits into from
Feb 9, 2018
Merged

Fix runner report and json newline #2601

merged 4 commits into from
Feb 9, 2018

Conversation

jquick
Copy link
Contributor

@jquick jquick commented Feb 9, 2018

There were a few issues with the new reporter work:

  1. The report function on the runner was not working properly with the new report types. This is used for the audit cookbook. This has been resolved.
  2. json/json-min were sending a newline at the end of their report. This has been resolved.

jquick added 3 commits February 9, 2018 10:59
@jquick
This fixes the audit issue expecting a report hash output.
1456045 
Signed-off-by: Jared Quick <jquick@chef.io>
@jquick
Add log format tests.
fc99ec5 
Signed-off-by: Jared Quick <jquick@chef.io>
@jquick
Fix json-config format not overriding reporter.
b5b0713 
Signed-off-by: Jared Quick <jquick@chef.io>
@jquick jquick requested a review from a team as a code owner February 9, 2018 18:09
@jquick
Remove ending newline from json reports.
69f6e4e 
Signed-off-by: Jared Quick <jquick@chef.io>
@jquick jquick force-pushed the jq/add_generic_report_output branch from 8b959d4 to 69f6e4e Compare February 9, 2018 18:15
chris-rock
chris-rock approved these changes Feb 9, 2018
View reviewed changes
Copy link
Contributor

@chris-rock chris-rock left a comment

Choose a reason for hiding this comment

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

Thank you @jquick

clintoncwolfe
clintoncwolfe reviewed Feb 9, 2018
View reviewed changes
Copy link
Contributor

@clintoncwolfe clintoncwolfe left a comment

Choose a reason for hiding this comment

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

My only comment is that I wish there were clear tests reproducing the regression, and showing that the change fixes it.

@jquick
Copy link
Contributor Author

jquick commented Feb 9, 2018 •
edited
Loading

Tests were added to confirm the regressions do not happen again. We capture the report output without newlines for the json. Tests were also added around the new report output to confirm its working as expected.

@jquick jquick merged commit 9420204 into master Feb 9, 2018
@jquick jquick deleted the jq/add_generic_report_output branch February 15, 2018 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clintoncwolfe clintoncwolfe clintoncwolfe left review comments

@miah miah miah approved these changes

@chris-rock chris-rock chris-rock approved these changes

@jerryaldrichiii jerryaldrichiii jerryaldrichiii approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jquick @miah @clintoncwolfe @chris-rock @jerryaldrichiii @adamhjk