-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Support for DependabotAlert APIs #2879
Conversation
The Dependabot Alert API takes its information from the dependency graph. However, there is no explicit dependency graph endpoints available via GitHub's REST API. Included "subclasses" which DependabotAlert is an aggregate of.
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2879 +/- ##
==========================================
- Coverage 96.70% 96.69% -0.01%
==========================================
Files 142 147 +5
Lines 14610 14848 +238
==========================================
+ Hits 14129 14358 +229
- Misses 481 490 +9 ☔ View full report in Codecov by Sentry. |
@EnricoMi thanks for taking a look at the reported enhancement request. When you get a chance, can you give a quick 👀 on the few changed files in this PR? I've added the list of tasks I will chip away at as part of adding this support but wanted to ensure I was using the correct patterns. I also reviewed the Contributing guide which helped immensely to get the first test case recorded and written 👍 In the meantime, I'll adapt the new class using |
DependabotAlert includes a mix of similar properties to RepositoryAdvisory, GlobalAdvisory and AdvisoryBase. However, there are certain fields that have different data so we need separate types for them. add remaining attributes for DependabotAlert
…Repository move vulnerabilities out of AdvisoryBase, remove typeddicts
2309195
to
55af3f8
Compare
github/Organization.py
Outdated
if is_defined(state): | ||
assert isinstance(state, str), state | ||
assert state in allowed_states, f"State can be one of {', '.join(allowed_states)}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd simplify this pattern to
if is_defined(state): | |
assert isinstance(state, str), state | |
assert state in allowed_states, f"State can be one of {', '.join(allowed_states)}" | |
assert state in allowed_states + [NotSet], f"State can be one of {', '.join(allowed_states)}" |
github/Organization.py
Outdated
if is_defined(state): | ||
assert isinstance(state, str), state | ||
assert state in allowed_states, f"State can be one of {', '.join(allowed_states)}" | ||
url_parameters["state"] = state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd replace the
if is_defined(state):
url_parameters["state"] = state
...
pattern with
url_parameters = NotSet.remove_unset_items(
{
"state": state,
"severity": severity,
"ecosystem": ecosystem,
"package": package,
"scope": scope,
"sort": sort,
"direction": direction,
}
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, applied the same for Repository.get_dependabot_alerts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks for this high-quality PR! This is ready to go, right? |
Yep, good to go! Thanks @EnricoMi |
Your PR was really great, I could use some help reviewing PRs in PyGithub. Is there a chance that from time to time you could have a look at open PRs and guide less experienced authors towards an approvable state? |
[](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [PyGithub](https://togithub.com/pygithub/pygithub) | `==2.1.1` -> `==2.2.0` | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>pygithub/pygithub (PyGithub)</summary> ### [`v2.2.0`](https://togithub.com/PyGithub/PyGithub/releases/tag/v2.2.0) [Compare Source](https://togithub.com/pygithub/pygithub/compare/v2.1.1...v2.2.0) #### Breaking Changes The `github.Comparison.Comparison` instance returned by `Repository.compare` provides a `commits` property that used to return a `list[github.Commit.Commit]`, which has now been changed to `PaginatedList[github.Commit.Commit]`. This breaks user code that assumes a `list`: ```python commits = repo.compare("v0.6", "v0.7").commits no_of_commits = len(commits) # will raise a TypeError ``` This will raise a `TypeError: object of type 'PaginatedList' has no len()`, as the returned `PaginatedList` does not support the `len()` method. Use the `totalCount` property instead: ```python commits = repo.compare("v0.6", "v0.7").commits no_of_commits = commits.totalCount ``` #### New features - Add support to call GraphQL API #### Improvements - Add parent_team_id, maintainers and notification_setting for creating and updating teams. by [@​Cheshirez](https://togithub.com/Cheshirez) in [https://github.com/PyGithub/PyGithub/pull/2863](https://togithub.com/PyGithub/PyGithub/pull/2863) - Add support for issue reactions summary by [@​smuzaffar](https://togithub.com/smuzaffar) in [https://github.com/PyGithub/PyGithub/pull/2866](https://togithub.com/PyGithub/PyGithub/pull/2866) - Support for DependabotAlert APIs by [@​coopernetes](https://togithub.com/coopernetes) in [https://github.com/PyGithub/PyGithub/pull/2879](https://togithub.com/PyGithub/PyGithub/pull/2879) - Derive GraphQL URL from base_url by [@​EnricoMi](https://togithub.com/EnricoMi) in [https://github.com/PyGithub/PyGithub/pull/2880](https://togithub.com/PyGithub/PyGithub/pull/2880) - Make `Repository.compare().commits` return paginated list by [@​EnricoMi](https://togithub.com/EnricoMi) in [https://github.com/PyGithub/PyGithub/pull/2882](https://togithub.com/PyGithub/PyGithub/pull/2882) - Add missing branch protection fields by [@​treee111](https://togithub.com/treee111) in [https://github.com/PyGithub/PyGithub/pull/2873](https://togithub.com/PyGithub/PyGithub/pull/2873) - Add `include_all_branches` to `create_repo_from_template` of `AuthenticatedUser` and `Organization` by [@​janssonoskar](https://togithub.com/janssonoskar) in [https://github.com/PyGithub/PyGithub/pull/2871](https://togithub.com/PyGithub/PyGithub/pull/2871) - Add and update organisation dependabot secrets by [@​mohy01](https://togithub.com/mohy01) in [https://github.com/PyGithub/PyGithub/pull/2316](https://togithub.com/PyGithub/PyGithub/pull/2316) - Add missing params to `Organization.create_repo` by [@​tekumara](https://togithub.com/tekumara) in [https://github.com/PyGithub/PyGithub/pull/2700](https://togithub.com/PyGithub/PyGithub/pull/2700) - Update allowed values for `Repository` collaborator permissions by [@​flying-sheep](https://togithub.com/flying-sheep) in [https://github.com/PyGithub/PyGithub/pull/1996](https://togithub.com/PyGithub/PyGithub/pull/1996) - Support editing PullRequestReview by [@​ColasGael](https://togithub.com/ColasGael) in [https://github.com/PyGithub/PyGithub/pull/2851](https://togithub.com/PyGithub/PyGithub/pull/2851) - Update attributes after calling `PullRequestReview.dismiss` by [@​ColasGael](https://togithub.com/ColasGael) in [https://github.com/PyGithub/PyGithub/pull/2854](https://togithub.com/PyGithub/PyGithub/pull/2854) - Add `request_cve` on `RepositoryAdvisories` by [@​JLLeitschuh](https://togithub.com/JLLeitschuh) in [https://github.com/PyGithub/PyGithub/pull/2855](https://togithub.com/PyGithub/PyGithub/pull/2855) - Filter collaborators of a repository by permissions by [@​notmicaelfilipe](https://togithub.com/notmicaelfilipe) in [https://github.com/PyGithub/PyGithub/pull/2792](https://togithub.com/PyGithub/PyGithub/pull/2792) - Set pull request to auto merge via GraphQL API by [@​heitorpolidoro](https://togithub.com/heitorpolidoro) in [https://github.com/PyGithub/PyGithub/pull/2816](https://togithub.com/PyGithub/PyGithub/pull/2816) - Support Environment Variables and Secrets by [@​AndrewJDawes](https://togithub.com/AndrewJDawes) in [https://github.com/PyGithub/PyGithub/pull/2848](https://togithub.com/PyGithub/PyGithub/pull/2848) - Update workflow.get_runs & pullrequest.add_to_assignees function signature by [@​sd-kialo](https://togithub.com/sd-kialo) in [https://github.com/PyGithub/PyGithub/pull/2799](https://togithub.com/PyGithub/PyGithub/pull/2799) - Add `GithubObject.last_modified_datetime` to have `last_modified` as a `datetime` by [@​chouetz](https://togithub.com/chouetz) in [https://github.com/PyGithub/PyGithub/pull/2772](https://togithub.com/PyGithub/PyGithub/pull/2772) - Add support for global advisories and unify some shared logic with repository advisories by [@​crimsonknave](https://togithub.com/crimsonknave) in [https://github.com/PyGithub/PyGithub/pull/2702](https://togithub.com/PyGithub/PyGithub/pull/2702) - Add `internal` as valid Repository visibility value by [@​AndrewJDawes](https://togithub.com/AndrewJDawes) in [https://github.com/PyGithub/PyGithub/pull/2806](https://togithub.com/PyGithub/PyGithub/pull/2806) - Add support for issue comments reactions summary by [@​smuzaffar](https://togithub.com/smuzaffar) in [https://github.com/PyGithub/PyGithub/pull/2813](https://togithub.com/PyGithub/PyGithub/pull/2813) #### Bug Fixes - Add a bunch of missing urllib.parse.quote calls by [@​ExplodingCabbage](https://togithub.com/ExplodingCabbage) in [https://github.com/PyGithub/PyGithub/pull/1976](https://togithub.com/PyGithub/PyGithub/pull/1976) - Fix Variable and Secret url bugs by [@​AndrewJDawes](https://togithub.com/AndrewJDawes) in [https://github.com/PyGithub/PyGithub/pull/2835](https://togithub.com/PyGithub/PyGithub/pull/2835) #### Maintenance - Update the class name for NetrcAuth in the examples by [@​vinnybod](https://togithub.com/vinnybod) in [https://github.com/PyGithub/PyGithub/pull/2860](https://togithub.com/PyGithub/PyGithub/pull/2860) - Move build to PEP517 by [@​trim21](https://togithub.com/trim21) in [https://github.com/PyGithub/PyGithub/pull/2800](https://togithub.com/PyGithub/PyGithub/pull/2800) - Use new type assert functions in `Repository` by [@​trim21](https://togithub.com/trim21) in [https://github.com/PyGithub/PyGithub/pull/2798](https://togithub.com/PyGithub/PyGithub/pull/2798) - PyTest: Move config to pyproject.toml by [@​Borda](https://togithub.com/Borda) in [https://github.com/PyGithub/PyGithub/pull/2859](https://togithub.com/PyGithub/PyGithub/pull/2859) - codespell: ignore-words-list by [@​Borda](https://togithub.com/Borda) in [https://github.com/PyGithub/PyGithub/pull/2858](https://togithub.com/PyGithub/PyGithub/pull/2858) - Improve fix-headers.py script by [@​EnricoMi](https://togithub.com/EnricoMi) in [https://github.com/PyGithub/PyGithub/pull/2728](https://togithub.com/PyGithub/PyGithub/pull/2728) - Remove dependency on python-dateutil by [@​lazka](https://togithub.com/lazka) in [https://github.com/PyGithub/PyGithub/pull/2804](https://togithub.com/PyGithub/PyGithub/pull/2804) - CI: update precommit & apply by [@​Borda](https://togithub.com/Borda) in [https://github.com/PyGithub/PyGithub/pull/2600](https://togithub.com/PyGithub/PyGithub/pull/2600) - Docs: Fix parameter order according to Version 2.1.0 by [@​nad182](https://togithub.com/nad182) in [https://github.com/PyGithub/PyGithub/pull/2786](https://togithub.com/PyGithub/PyGithub/pull/2786) - Add missing GitHub classes to docs by [@​EnricoMi](https://togithub.com/EnricoMi) in [https://github.com/PyGithub/PyGithub/pull/2783](https://togithub.com/PyGithub/PyGithub/pull/2783) - CI: Fix mypy error by ignoring override by [@​EnricoMi](https://togithub.com/EnricoMi) in [https://github.com/PyGithub/PyGithub/pull/2779](https://togithub.com/PyGithub/PyGithub/pull/2779) **Full Changelog**: PyGithub/PyGithub@v2.1.1...v2.2.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/lettuce-financial/github-bot-signed-commit). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xNTMuMiIsInVwZGF0ZWRJblZlciI6IjM3LjE1My4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->
@EnricoMi Apologies for the late reply! Absolutely, I am available to help with triage & reviewing PRs to support the library. Feel free to assign me PRs or issues and don't hesitate to send me an email if you need to discuss anything else. |
Fixes #2872
Related #2347 (Dependabot alerts are generated from the underlying dependency graph)
This PR includes the following to bring in all the supported operations for the Dependabot alert APIs:
get_dependabot_alert
for Repositoryget_dependabot_alerts
for Repository using PaginatedListupdate_dependabot_alert
for Repositoryget_dependabot_alerts
for Organization using PaginatedListAddGitHub's endpoint doesn't work from my limited testingget_dependabot_alerts
for Enterprise using PaginatedListAdvisoryVulnerability
fromAdvisoryBase
and moved that specific attribute intoRepositoryAdvisory
&GlobalAdvisory
. Most of the vulnerability information is similar between advisories and Dependabot alerts but there were a few differences in the JSON and I decided to split them up from the base class.Details on my refactoring amongst the other security-related modules/classes:
After reviewing the existing types for AdvisoryBase, GlobalAdvisory, RepositoryAdvisory and AdvisoryVulnerability, it's clear that the Dependabot alert API looks very similar to those other endpoints with some annoyingly minor differences. For example:
dependency.package
for a Dependabot alert is identical toAdvisoryVulnerabilityPackage
but has additional fields (manifest_path
andscope
) so I couldn't reuse the entirety ofAdvisoryVulnerability
. There's likely a place for a Union type but I didn't have a good sense on the best way to write that.vulnerabilities
list for a Dependabot alert is slightly different than the Repository/GlobalAdvisoryvulnerabilities
(the former includesseverity
andfirst_patched_version
object, the latter usespatched_versions
& containsvulnerable_functions
)references
are their own list of dicts for Dependabot alerts (with a single field called "url" 🤦) but are just a list of strings for GlobalAdvisoryInstead of monkeying around with too much of the existing modules to try & find all the common attributes and create new base modules, I kept most of the Dependabot specific API types as separate modules & classes. I was able to reuse
AdvisoryVulnerabilityPackage
.