Skip to content

Conversation

@rndev15
Copy link
Contributor

@rndev15 rndev15 commented Dec 5, 2024

Hey there, in this PR I try to resolve #195

@rndev15 rndev15 requested a review from kunalnagar as a code owner December 5, 2024 14:01
@rndev15
Copy link
Contributor Author

rndev15 commented Dec 9, 2024

Good day @kunalnagar , could you please take a look into this PR?

@rndev15
Copy link
Contributor Author

rndev15 commented Dec 13, 2024

Hey @kunalnagar any chance to take a look into?

@kunalnagar
Copy link
Member

@rndev15 - I do like this idea of being able to mute/ignore certain dependencies that one does not care about. Have you looked at if the dependabot API allows passing in a list of packages to ignore instead of manually doing a .filter() on the alerts?

@rndev15
Copy link
Contributor Author

rndev15 commented Dec 18, 2024

hey @kunalnagar I took a look into docs for listAlertsForRepo and there is no ability to filter dependencies on api side. So I decided to filter response itself.

Copy link
Member

@kunalnagar kunalnagar left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Couple of things:

  1. Could you make sure that the CI passes with your changes?
  2. Can you post a few screenshots of how the action behaves with this ignore_packages field? Null, empty values etc and let's test this out thoroughly

# slack_webhook: ${{ secrets.SLACK_WEBHOOK }}
# severity: low,medium
# ecosystem: npm,rubygems
# ignore_dependencies: lodash,devise
Copy link
Member

Choose a reason for hiding this comment

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

Let's call it ignore_packages since GitHub calls them packages instead of dependencies

action.yml Outdated
ecosystem:
description: 'A comma-separated list of ecosystems. If specified, only alerts for these ecosystems will be returned.'
ignore_dependencies:
description: 'A comma-separated list of dependencies to ignore. If specified, these dependencies will not be alerted.'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description: 'A comma-separated list of dependencies to ignore. If specified, these dependencies will not be alerted.'
description: 'A comma-separated list of package names. If specified, alerts for these packages will be ignored.'

repositoryOwner: string,
severity: string,
ecosystem: string,
ignoreDependencies: string[],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ignoreDependencies: string[],
ignorePackages: string[],

Comment on lines 33 to 40
const alerts: Alert[] = response
.data
.filter((dependabotAlert) =>
!ignoreDependencies.includes(dependabotAlert.security_vulnerability.package.name)
)
.map((dependabotAlert) =>
toRepositoryAlert(dependabotAlert, repositoryName, repositoryOwner),
)
Copy link
Member

Choose a reason for hiding this comment

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

  1. Do you mind running the lint task and see if it passes? I feel like your editor may not be set up correctly to format the code automatically.
  2. We only need to filter the alerts if the ignorePackages field contains a value. We should check for that, and only filter if required.

Comment on lines 65 to 72
const alerts: Alert[] = response
.data
.filter((dependabotOrgAlert) =>
!ignoreDependencies.includes(dependabotOrgAlert.security_vulnerability.package.name)
)
.map((dependabotOrgAlert) =>
toOrgAlert(dependabotOrgAlert),
)
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above

Comment on lines 97 to 104
const alerts: Alert[] = response
.data
.filter((dependabotEnterpriseAlert) =>
!ignoreDependencies.includes(dependabotEnterpriseAlert.security_vulnerability.package.name)
)
.map((dependabotEnterpriseAlert) =>
toEnterpriseAlert(dependabotEnterpriseAlert),
)
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above

src/main.ts Outdated
Comment on lines 43 to 44
getInput('ignore_dependencies') || ''
).split(',').map((str: string) => str.trim())
Copy link
Member

Choose a reason for hiding this comment

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

Let's pass the value here directly using getInput and do the transform inside the logic where we check if this field has values. We don't need to do this transform if no values exist.

@rndev15 rndev15 force-pushed the allow-ignore-deps branch 10 times, most recently from 4ea676e to 811c916 Compare January 21, 2025 13:24
@rndev15
Copy link
Contributor Author

rndev15 commented Jan 21, 2025

Good day, @kunalnagar

With the help of my colleague we made some changes to original PR:

  1. Splitted fetch*Alerts to different files
  2. Allow to specify not only package but CVE to ignore (see example in action.yml)

Here is the example of how feature works (I blurred repo & org name). On first slack message we can see how code works with ignore_packages equals to ''. Second message: I set ignore_packages to jquery-ui-rails#CVE-2022-31160,bootstrap-sass

Снимок экрана 2025-01-21 в 16 17 20(2)

@rndev15 rndev15 requested a review from kunalnagar January 27, 2025 09:09
@rndev15
Copy link
Contributor Author

rndev15 commented Jan 31, 2025

Good day, @kunalnagar
any chance to take a look, please?

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.

Feature request: Allow to ignore specific dependencies

2 participants