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

Add --status flag #1030

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add --status flag #1030

wants to merge 7 commits into from

Conversation

mtpereira
Copy link

Fixes #918

This change adds the --status flag as described on the referenced issue, so that kube-bench will only log tests results with a check.State contained in the --status flag. If the flag is not passed, all messages are logged.
This does not affect the summary output.

@CLAassistant
Copy link

CLAassistant commented Oct 23, 2021

CLA assistant check
All committers have signed the CLA.

@mtpereira
Copy link
Author

Hi! 👋 I'm looking at this issue in the context of Hacktoberfest 2021.

If you think this PR has a meaningful change, and it might not be merged/approved before the end of the month, could you please label it with hacktoberfest-accepted?

Thank you!

@mtpereira
Copy link
Author

📝 I've just noticed I did not do any sort of error handling/input check on the --status flag. I'll handle that as well, although it would be great if I could get some feedback before that, to ensure I'm on the right track. 🙏

@codecov
Copy link

codecov bot commented Oct 27, 2021

Codecov Report

Merging #1030 (5ed6c78) into main (dd68e85) will increase coverage by 0.18%.
The diff coverage is 71.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1030      +/-   ##
==========================================
+ Coverage   63.56%   63.75%   +0.18%     
==========================================
  Files          14       14              
  Lines        1905     1948      +43     
==========================================
+ Hits         1211     1242      +31     
- Misses        633      645      +12     
  Partials       61       61              
Impacted Files Coverage Δ
check/controls.go 75.11% <0.00%> (-4.49%) ⬇️
cmd/common.go 57.58% <92.50%> (+3.24%) ⬆️
cmd/root.go 34.50% <100.00%> (+0.46%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd68e85...5ed6c78. Read the comment docs.

@yoavrotems
Copy link
Contributor

Thanks for contributing, First I will check about the hacktoberfest.
Secondly we would like --status to change the complete output, because the use of it will be to better know what tests in what status especially when we would support more statuses, for example if I would like to see what I need to fix in my system and thats it I will run:
kube-bench --status="FAIL"
And would expect only the failed tests to be outputted with the remediation and the summary of the different parts.
(About the json I'm reluctant from changing the output since it is very easy to run manipulation on json data so maybe we should keep it as informative as possible)

@yoavrotems yoavrotems self-requested a review October 27, 2021 10:23
@mtpereira
Copy link
Author

Hi @yoavrotems ! 👋

I'm not sure I understood what you meant with your comment on what the output should be, so I'll try to clarify it. 🙏

Do you mean that:

  1. when the --status flag is set, all of the stdout should be filtered, including summary and report;
  2. Or do you mean that other methods of outputting should also be affected, with the exception of JSON, as you've mentioned?

Thanks for the clarification!

@mtpereira
Copy link
Author

mtpereira commented Oct 30, 2021

To make this more explicit and hopefully easier to communicate, here is the current output I get from integration tests when I modify them to pass --status=FAIL: https://gist.github.com/mtpereira/12e0acb97a209b2ae72389d3a640ca28

As you can see, the whole output is only about the checks that FAILed.

Is this not what we want?

@yoavrotems
Copy link
Contributor

Hey you were right in the first part

Hi @yoavrotems ! 👋

I'm not sure I understood what you meant with your comment on what the output should be, so I'll try to clarify it. 🙏

Do you mean that:

  1. when the --status flag is set, all of the stdout should be filtered, including summary and report;
  2. Or do you mean that other methods of outputting should also be affected, with the exception of JSON, as you've mentioned?

Thanks for the clarification!

About the second part all of the output manipulation flags --noremediations --nosummary --noresults effects the stdout only and not the json.
And yes it should all co-exist I don't see a reason why not using --noremediations and --status=FAIL

I saw the output you shared status wise it seems good, but I would skip printing remediation for not printed tests like you have with 3.1.1 3.2.1 3.2.2 those tests all are WARN so it not printed but the remediation do.

If someone wanted to see all fails tests the it remediation I guess that WARN tests remediation is not relevant here. if so you should run --status="FAIL,WARN"

wdyt?

@mtpereira
Copy link
Author

@yoavrotems Got it, that makes sense! I'll give it a look. 👌

Added a Summary.Results method to make it possible to get a specific
State result.
@mtpereira
Copy link
Author

Hello @yoavrotems 👋

How is this looking now? https://gist.github.com/mtpereira/12e0acb97a209b2ae72389d3a640ca28/revisions

📝 The code needs some refactoring, it is still not ready, but tests are 🟢 .

@yoavrotems
Copy link
Contributor

Hey looks better the output that not including remediation when its not printing the test is great!
This PR is great and part from a bigger change that we want to see, I think you should take in consideration this PR #1035

@mtpereira
Copy link
Author

Hey looks better the output that not including remediation when its not printing the test is great!

This PR is great and part from a bigger change that we want to see, I think you should take in consideration this PR #1035

Hi! 👋

Oh this might be tricky to handle! 😅 How do you suggest we go about this? Shall we merge one PR first and then proceed with the other? I'm fine either way. 👍

@yoavrotems
Copy link
Contributor

Let me check it and return to you on how we are going to accept the other PR because we might take as a flag to not break old behaviors

@yoavrotems
Copy link
Contributor

Hey @mtpereira We would accept the #1035 PR but we will do it as a flag for now, the default will be using all of the statuses, but with flag --enhancedoutput=false to allow backward compatibility, so I think that this PR will need to support filtering all of the statuses.
One thing we need to decide is the behavior when printing unused flag, for example:
./kube-bench --enhancedoutput=false --status="WARN,MANU"

I think that we can do in one of the two ways,

  1. When use flag status check what filtering is applying and if using new status with enhancedoutput=false, raise error and not running. --> kube-bench can't output non-supported status, either remove $STATUS from --status, or set --enhancedoutput=true P.S should also support for something like this one trying none exist status.

  2. When trying to filter out none exist flag just not print it since there isn't any $STATUS in the output, same as I would run --status="FAIL" and none of the tests failed.

I think that the first option is the best but open to suggestions.
WDYT?

@mtpereira
Copy link
Author

Hello @yoavrotems ! 👋

Option 1 seems better to me as well! I'll give it a look once I have some spare time. 🙇

@yoavrotems yoavrotems added this to the v0.7.0 milestone Jan 12, 2022
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.

New flag! --status
3 participants