-
Notifications
You must be signed in to change notification settings - Fork 201
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 curl advisories importer #1439
Add curl advisories importer #1439
Conversation
@TG1999 please let me know if there are any further changes and everything is correct or not. |
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.
@ambuj-1211 This is a good start but try to make use of all available data ex: details, CWE , ..
on it |
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.
@ambuj-1211 Nice work! just a few nits for your consideration.
@ambuj-1211 please add a curl improver to the ex: https://github.com/nexB/vulnerablecode/blob/main/vulnerabilities/improvers/valid_versions.py#L472 |
|
sorry it was a mistake and I fixed it |
@ziadhany please review the changes and do let me know. |
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.
Great work! please fix those failing tests then squash the commits
@ziadhany I made the changes, should I also update my branch? |
@ambuj-1211 Absolutely! Once all the changes are in place, squashing them into a single commit before merging will create a cleaner history for everyone. to fix the test update the |
2f84bec
to
5ae6ac0
Compare
@ziadhany some checks are not successfull but they are because of openssl url do I need to change it to some thing, I am asking because it is not related to curl advisories. |
@ambuj-1211 No, this is the docs test ( not related to this pull request ) |
so this pr is good to go? |
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, just a minor nit for your consideration
vulnerabilities/tests/test_data/curl/expected_curl_advisory_output3.json
Outdated
Show resolved
Hide resolved
f2729d8
to
c9ebb70
Compare
- added curl importer - added tests for curl importer Signed-off-by: ambuj <[email protected]>
c9ebb70
to
24b5eaa
Compare
@ziadhany check the files now I have changed the files and squashed the commits, I think now the history and everything is clean and good to go, please let me know if there are any more nits. |
a5b0752
to
cb4ca3e
Compare
…l importer. Signed-off-by: ambuj <[email protected]>
cb4ca3e
to
fcd4c0c
Compare
@ambuj-1211 please resolve conflicts on this PR |
@ziadhany is this PR good to merge, if yes please merge this in. Thanks! |
@ambuj-1211 please see tests are failing |
Signed-off-by: ambuj <[email protected]>
corrected the code now check @TG1999 |
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.
@ambuj-1211 LGTM, Just remove the unused importer and squash the commits.
@TG1999 please have a look on the log file |
@ambuj-1211 thanks! merged! |
This pr resolves the issue #1166.
The advisories were added properly and the site was working fine. I also registered the importer in the init.py file.