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

feat(general): add flag for summary position #3497

Merged
merged 4 commits into from
Sep 13, 2022

Conversation

marynaKK
Copy link
Contributor

@marynaKK marynaKK commented Sep 12, 2022

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Description

Fixes #3222

Description

Added a flag to enable changing the summary position within the report. Default behavior didn't change - summary at the top. adding the flag --summary-position-bottom will move it to the bottom.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my feature, policy, or fix is effective and works
  • New and existing tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@arielkru
Copy link
Contributor

Very nice @marynaKK !
I see James suggested the default position to be at the bottom and it makes sense to me, shouldn't we implement it that way?
#3222 (comment)

@marynaKK
Copy link
Contributor Author

@arielkru You make sense :) I thought to preserve the current behavior, the flag default is top, and if it is used the summary moves to the bottom.

@marynaKK marynaKK requested a review from nimrodkor September 12, 2022 13:37
Copy link
Contributor

@nimrodkor nimrodkor left a comment

Choose a reason for hiding this comment

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

Is there a test for this @marynaKK ? If not - can we write one?

@gruebel
Copy link
Contributor

gruebel commented Sep 12, 2022

if we change the default behaviour, then we should mark this PR as a breaking change -> break(general): ...

Copy link
Contributor

@nimrodkor nimrodkor left a comment

Choose a reason for hiding this comment

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

No tests in that area then... Only update the docs:

# CLI Command Reference

@gruebel This is not a breaking change, only if you set the arg do you get a different behavior

Copy link
Contributor

@gruebel gruebel left a comment

Choose a reason for hiding this comment

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

nice, great job on the first PR 💪

checkov/main.py Outdated Show resolved Hide resolved
@gruebel
Copy link
Contributor

gruebel commented Sep 13, 2022

@nimrodkor that's why I wrote, if we change the default behaviour 🙂 if we don't, we don't need 😄

@marynaKK
Copy link
Contributor Author

@gruebel Your suggestion is more clear - I'll keep it in mind to keep things simpler :)
changed it.

@marynaKK marynaKK requested a review from gruebel September 13, 2022 13:23
Copy link
Contributor

@gruebel gruebel left a comment

Choose a reason for hiding this comment

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

great work ☕ 🍰

@marynaKK marynaKK merged commit ed8c376 into master Sep 13, 2022
@marynaKK marynaKK deleted the add_flag_summary_posotion branch September 13, 2022 14:15
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.

Move CLI output summary to the bottom of the output
4 participants