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.

Sign up for GitHub

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

Jump to bottom

feat: Allow to ignore specific dependencies #196

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: Allow to ignore specific dependencies #196

wants to merge 1 commit into from

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.

kunalnagar
kunalnagar reviewed Dec 18, 2024
View reviewed changes
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

.github/workflows/ci.yml
@@ -34,6 +34,7 @@ jobs:
# 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
@@ -43,6 +43,8 @@ inputs:
description: 'Comma separated list of severities. E.g. low,medium,high,critical (NO SPACES BETWEEN COMMA AND SEVERITY)'
ecosystem:
description: 'A comma-separated list of ecosystems. If specified, only alerts for these ecosystems will be returned.'
ignore_dependencies:
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
ignore_dependencies:
ignore_packages:

action.yml
@@ -43,6 +43,8 @@ inputs:
description: 'Comma separated list of severities. E.g. low,medium,high,critical (NO SPACES BETWEEN COMMA AND SEVERITY)'
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.'

src/fetch-alerts.ts
@@ -13,6 +13,7 @@ export const fetchRepositoryAlerts = async (
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[],

src/fetch-alerts.ts
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.

src/fetch-alerts.ts
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

src/fetch-alerts.ts
@@ -66,6 +78,7 @@ export const fetchEnterpriseAlerts = async (
enterprise: 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[],

src/fetch-alerts.ts
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
@@ -39,16 +39,20 @@ async function run(): Promise<void> {
const count = parseInt(getInput('count'))
const severity = getInput('severity')
const ecosystem = getInput('ecosystem')
const ignoreDependencies = (
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
const ignoreDependencies = (
const ignorePackages = (

src/main.ts
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kunalnagar kunalnagar kunalnagar left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature request: Allow to ignore specific dependencies
2 participants
@rndev15 @kunalnagar