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

Separate console logger for plugins #2837

Merged

Conversation

anderseknert
Copy link
Member

This allows logging to console for decisions and status (and possibly other use cases) without having to follow the generic --log-level.

Fixes #2733

Signed-off-by: Anders Eknert anders.eknert@bisnode.com

@anderseknert anderseknert force-pushed the Plugin-console-logger branch from 143808b to 0b6ad84 Compare October 28, 2020 21:46
Copy link
Member

@ashutosh-narkar ashutosh-narkar left a comment

Choose a reason for hiding this comment

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

It would be great to add some unit test coverage for this. For example we could set output on the plugin logger and check if the decisions/status are being logged.

@anderseknert
Copy link
Member Author

@ashutosh-narkar yeah I thought about that, but would it not essentially just be repeating the e2e test in a unit test format? I don't know too much about how codecov works, but does it not take all test types into account when calculating coverage?

@ashutosh-narkar
Copy link
Member

@ashutosh-narkar yeah I thought about that, but would it not essentially just be repeating the e2e test in a unit test format? I don't know too much about how codecov works, but does it not take all test types into account when calculating coverage?

Re. codecov, me neither 🤷‍♂️ . I was thinking about a test where we set the regular logger to say error and then test out the plugin logger does the right thing.

@anderseknert
Copy link
Member Author

That's pretty much what we do, no?

Oh well, I'll add a unit test for the status plugin since console logging seems untested there and thus we won't have to repeat the same test in another format. That should give us some codecov points too :)

@ashutosh-narkar
Copy link
Member

That's pretty much what we do, no?

Oh well, I'll add a unit test for the status plugin since console logging seems untested there and thus we won't have to repeat the same test in another format. That should give us some codecov points too :)

My bad I missed that line. Sounds good. Thanks !

@anderseknert anderseknert force-pushed the Plugin-console-logger branch from 0b6ad84 to c4b598f Compare October 28, 2020 23:29
This allows logging to console for decisions and status (and possibly other use cases) without having to follow the generic --log-level.

Fixes open-policy-agent#2733

Signed-off-by: Anders Eknert <anders.eknert@bisnode.com>
@anderseknert anderseknert force-pushed the Plugin-console-logger branch from c4b598f to 44cb167 Compare October 28, 2020 23:36
@anderseknert
Copy link
Member Author

Unit test added. While it's good to have nonetheless it didn't do much in terms of pleasing codecov. I'm honestly lost here as the change itself is minimal, and all lines added are tested as far as I can tell.

Copy link
Member

@ashutosh-narkar ashutosh-narkar left a comment

Choose a reason for hiding this comment

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

lgtm

@tsandall tsandall requested a review from patrick-east October 29, 2020 18:10
@tsandall
Copy link
Member

@patrick-east PTAL

@patrick-east patrick-east merged commit f7f793c into open-policy-agent:master Oct 30, 2020
@anderseknert anderseknert deleted the Plugin-console-logger branch October 30, 2020 21:30
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.

Decouple decision_logs.console from general log level
4 participants