Skip to content

Conversation

@nyg
Copy link
Contributor

@nyg nyg commented Feb 26, 2022

Changes

When querying Github's GraphQL API to retrieve vulnerability alerts, only retrieve those that are in OPEN state.

Context

Closes #14316

The GraphQL request to retrieve vulnerabilities currently ignores their state (OPEN, FIXED or DISMISSED).

Fixed or dismissed vulnerabilities should not be considered as renovate will try to create PRs for these vulnerabilities, when actually the dependencies are already fixed (or the owner has decided they should not be fixed). This will also propose the PRs in the Dashboard issue under "Other branches" or "Ignored or blocked".

It seems this state was added recently, which may explain why this issue was not noticed before.

https://docs.github.com/en/graphql/reference/enums#repositoryvulnerabilityalertstate
https://docs.github.com/en/graphql/overview/changelog (Enum added on Feb 16th, 2022)

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please tick one)

Note: I have not written any test for this as this is a feature of Github's GraphQL API. Let me know if I should still be writing a test for it.

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

This will potentially break GHE and needs tests.

@rarkins
Copy link
Contributor

rarkins commented Feb 28, 2022

If it's too complicated to add GHE version detection to this, it would be better to undo this change and add the appropriate filtering on our client side instead, so it works with all.

@viceice
Copy link
Member

viceice commented Feb 28, 2022

We already know if we on GHE, so check should be easy. We can enable filter on GHE when we know on which GHE version it's available later.

@rarkins
Copy link
Contributor

rarkins commented Feb 28, 2022

But the problem this is addressing will still exist on GHE in the meantime?

@nyg
Copy link
Contributor Author

nyg commented Feb 28, 2022

If it's too complicated to add GHE version detection to this, it would be better to undo this change and add the appropriate filtering on our client side instead, so it works with all.

Indeed, I think it would be a bit complicated to check for the GHE versions that support of not the new state field.

But then, how would you filter on the client side? Because if we add the state field to the query, then it's also going to fail with the GHE versions that don't support it.

One option I thought about, but don't know if it's doable, is schema introspection, i.e. to check if the state field is present or not, and depending on the result include or not the state field in the query.

But the problem this is addressing will still exist on GHE in the meantime?

From what I understand, yes.

@nyg nyg marked this pull request as draft March 1, 2022 21:48
@nyg nyg marked this pull request as ready for review March 3, 2022 20:13
@nyg nyg requested a review from viceice March 3, 2022 20:15
# Conflicts:
#	lib/modules/platform/github/__snapshots__/index.spec.ts.snap
@rarkins

This comment was marked as outdated.

@rarkins rarkins closed this Mar 25, 2022
@rarkins rarkins reopened this Mar 25, 2022
@rarkins
Copy link
Contributor

rarkins commented Mar 25, 2022

Reopening this, as I changed my mind :)

rarkins
rarkins previously approved these changes Mar 27, 2022
@rarkins rarkins requested review from viceice and removed request for viceice March 27, 2022 11:12
@rarkins rarkins enabled auto-merge (squash) March 27, 2022 11:13
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

auto-merge was automatically disabled March 27, 2022 13:48

Head branch was pushed to by a user without write access

rarkins
rarkins previously approved these changes Mar 27, 2022
@rarkins rarkins requested a review from viceice March 27, 2022 14:08
@rarkins rarkins enabled auto-merge (squash) March 27, 2022 14:08
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

Needs snapshot update

auto-merge was automatically disabled March 27, 2022 15:41

Head branch was pushed to by a user without write access

@rarkins rarkins enabled auto-merge (squash) March 27, 2022 16:58
@rarkins rarkins requested a review from viceice March 28, 2022 11:20
@rarkins rarkins disabled auto-merge March 28, 2022 12:16
@rarkins rarkins merged commit 18b884d into renovatebot:main Mar 28, 2022
@renovate-release
Copy link

🎉 This PR is included in version 32.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nyg nyg deleted the fix/ignore-fixed-dismissed-vuln-alerts branch March 29, 2022 18:43
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 29, 2022
rarkins added a commit that referenced this pull request Jun 7, 2022
@renovate-release
Copy link

🎉 This issue has been resolved in version 32.76.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants