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

Xblanchot/sca 942/fix commit range in ci #679

Merged
merged 1 commit into from
Aug 11, 2023

Conversation

xblanchot-gg
Copy link
Contributor

@xblanchot-gg xblanchot-gg commented Aug 9, 2023

This MR adds a util to get the previous state of the targeted branch in CI for github, and use it in sca scan ci.
It also removes the need of defining a custom env variable for PR in github CI. A custom env variable is still required for push event on github.
For now, SCA and IaC CI scans work only with github and gitlab CIs.
All other CIs will come in a next PR.

@xblanchot-gg xblanchot-gg requested a review from GG-HH August 9, 2023 13:19
@xblanchot-gg xblanchot-gg force-pushed the xblanchot/SCA-942/fix_commit_range_in_ci branch from e6f93d9 to e00b33d Compare August 10, 2023 10:25
@xblanchot-gg xblanchot-gg force-pushed the xblanchot/SCA-942/fix_commit_range_in_ci branch 3 times, most recently from db6164c to 1a4b9d6 Compare August 10, 2023 12:27
ggshield/core/git_hooks/ci/commit_range.py Outdated Show resolved Hide resolved
ggshield/core/git_hooks/ci/previous_commit.py Outdated Show resolved Hide resolved
ggshield/core/git_hooks/ci/previous_commit.py Show resolved Hide resolved
ggshield/core/git_hooks/ci/previous_commit.py Outdated Show resolved Hide resolved
ggshield/core/git_hooks/ci/previous_commit.py Outdated Show resolved Hide resolved
ggshield/core/git_hooks/ci/previous_commit.py Show resolved Hide resolved
@xblanchot-gg xblanchot-gg force-pushed the xblanchot/SCA-942/fix_commit_range_in_ci branch from 1a4b9d6 to d5a5115 Compare August 10, 2023 13:04
ggshield/cmd/iac/scan/ci.py Outdated Show resolved Hide resolved
ggshield/cmd/sca/scan/ci.py Outdated Show resolved Hide resolved
ggshield/core/git_hooks/ci/previous_commit.py Show resolved Hide resolved
ggshield/core/git_hooks/ci/previous_commit.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@agateau-gg agateau-gg 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 overall, just minor remarks.

The single-commit message is deceiving: I expect a commit whose message is "Add an util to do foo", to mainly add a function and use it, not split a module in 4 and introduce many functions.

Doing the split in a separate commit would have helped tracking the real changes.

ggshield/core/git_hooks/ci/previous_commit.py Outdated Show resolved Hide resolved
ggshield/cmd/iac/scan/ci.py Outdated Show resolved Hide resolved
ggshield/cmd/iac/scan/ci.py Outdated Show resolved Hide resolved
ggshield/cmd/iac/scan/ci.py Outdated Show resolved Hide resolved
ggshield/cmd/sca/scan/ci.py Outdated Show resolved Hide resolved
ggshield/core/git_hooks/ci/commit_range.py Show resolved Hide resolved
Copy link
Collaborator

@GG-HH GG-HH left a comment

Choose a reason for hiding this comment

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

Nicely done! Thanks a lot for this work!

ggshield/core/git_hooks/ci/previous_commit.py Outdated Show resolved Hide resolved
ggshield/core/git_hooks/ci/previous_commit.py Show resolved Hide resolved
@xblanchot-gg xblanchot-gg force-pushed the xblanchot/SCA-942/fix_commit_range_in_ci branch from 14476f4 to dffd627 Compare August 11, 2023 09:43
Copy link
Collaborator

@agateau-gg agateau-gg left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

tests/unit/cmd/sca/test_ci.py Outdated Show resolved Hide resolved
@xblanchot-gg xblanchot-gg force-pushed the xblanchot/SCA-942/fix_commit_range_in_ci branch from dffd627 to ef98d62 Compare August 11, 2023 12:05
@xblanchot-gg xblanchot-gg merged commit cabdffd into main Aug 11, 2023
22 checks passed
@xblanchot-gg xblanchot-gg deleted the xblanchot/SCA-942/fix_commit_range_in_ci branch August 11, 2023 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants