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 basic pull request comment alert type #5133

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

eleftherias
Copy link
Contributor

@eleftherias eleftherias commented Dec 4, 2024

Summary

This adds a very basic pull request comment alert type.
When an evaluation fails and alerts are enabled, this alert type will comment on a PR.

This does not include line comments, only a single review comment, which is hardcoded in the ruletype definition.

Ref #5117

See "Testing" below for instruction on how to try it.

Change Type

Mark the type of change your PR introduces:

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

This is still a work in progress, but the happy path works.

  1. Write a new ruletype using the pull_request_review as the alert type, for example:
---
version: v1
release_phase: alpha
type: rule-type
name: pr_example_basic
display_name: Basic PR example
short_failure_message: PR example failed
severity:
  value: medium
context:
  provider: github
description: |
  Basic PR review example
guidance: |
  Write a better PR reviewer
def:
  in_entity: pull_request
  param_schema:
    properties: {}
  rule_schema: {}
  ingest:
    type: diff
    diff:
      type: full
  eval:
    type: homoglyphs
    homoglyphs:
      type: mixed_scripts
  alert:
    type: pull_request_comment
    pull_request_comment:
      review_message: "This is a message from the PR review alert type"
  1. Write a profile that uses the ruletype, with alerts turned on
---
# Profile ensuring that repository settings are configured
version: v1
type: profile
name: custom-profile
display_name: Repository Security Custom
context:
  provider: github
alert: "on"
remediate: "off"
pull_request:
  - type: pr_example_basic
    def: {}

  1. Apply the ruletype and profile
minder ruletype create -f rule-types/basic_pr_review_example.yaml
minder profile apply -f profile.yaml
  1. Trigger an evaluation failure, in this case I created a PR that adds mixed scripts, see Add mixed scripts eleftherias/demo-repo-go#10

  2. Minder will comment on the PR with the message set in the ruletype definition

Screenshot 2024-12-04 at 13 59 10

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

@eleftherias eleftherias changed the title Add basic pull request review alert type Add basic pull request comment alert type Dec 4, 2024
@coveralls
Copy link

coveralls commented Dec 4, 2024

Coverage Status

coverage: 55.168% (-0.03%) from 55.194%
when pulling fb980a2 on eleftherias:pr-alert-type-basic
into c2d9126 on mindersec:main.

@eleftherias eleftherias marked this pull request as ready for review December 5, 2024 09:19
@eleftherias eleftherias requested a review from a team as a code owner December 5, 2024 09:19
Number int
Metadata *alertMetadata
prevStatus *db.ListRuleEvaluationsByProfileIdRow
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Right, so... If I understand correctly, this are the parameters to create a PR regardless of the underlying driver, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right

internal/engine/actions/alert/alert.go Show resolved Hide resolved
return nil, fmt.Errorf("no PR comment ID provided: %w", enginerr.ErrActionTurnedOff)
}

_, err := alert.gh.DismissReview(ctx, params.Owner, params.Repo, params.Number, *params.Metadata.ReviewID,
Copy link
Member

Choose a reason for hiding this comment

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

Do I read this correctly that the ReviewID is what would differentiate alerts/reviews between different ruletypes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because at this point we do one review/comment per ruletype. Once we combine all Minder comments into a single review, then we will only have one reviewId per entity

@eleftherias eleftherias merged commit ff1fca9 into mindersec:main Dec 6, 2024
27 checks passed
@eleftherias eleftherias deleted the pr-alert-type-basic branch December 6, 2024 08:03
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.

4 participants