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.

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

Collect go vulnerabilities from github api #578

Merged
merged 17 commits into from
Feb 15, 2022
Merged

Conversation

sify21
Copy link
Contributor

@sify21 sify21 commented Nov 17, 2021

try to fix #466

@sify21
Copy link
Contributor Author

sify21 commented Nov 17, 2021

fetching release_date for each version is very slow, commenting out those lines will make it a lot faster

@pombredanne
Copy link
Member

@sify21 Thank you ++ for this! Let me review this in details.

Copy link
Collaborator

@sbs2001 sbs2001 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 PR @sify21 .

Most of the stuff looks very good. Thanks for updating the type hints btw.

As an improvement in clarity and readability, maybe restructure the GoproxyVersionAPI such that:

The fetch method only does one thing, it should only fetch and update the cache. It shouldn't

  • Do low level stuff on modifying the obtained version_info.
  • Construct the api url.

Maybe create separte functions/methods for above two.

Also some unittests would be helpful.

vulnerabilities/package_managers.py Outdated Show resolved Hide resolved
vulnerabilities/package_managers.py Outdated Show resolved Hide resolved
vulnerabilities/package_managers.py Outdated Show resolved Hide resolved
vulnerabilities/package_managers.py Outdated Show resolved Hide resolved
vulnerabilities/package_managers.py Show resolved Hide resolved
vulnerabilities/package_managers.py Outdated Show resolved Hide resolved
vulnerabilities/package_managers.py Outdated Show resolved Hide resolved
vulnerabilities/package_managers.py Outdated Show resolved Hide resolved
vulnerabilities/package_managers.py Outdated Show resolved Hide resolved
vulnerabilities/package_managers.py Outdated Show resolved Hide resolved
Signed-off-by: 司芳源 <[email protected]>
Copy link
Collaborator

@sbs2001 sbs2001 left a comment

Choose a reason for hiding this comment

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

Thanks. This would be good to merge after

  1. Adding some comments on as suggested inline. Without prior knowledge it's hard to understand the intent of the code.
  2. Add some test cases or just examples in a doc string trim_url_path and escape_path methods.

vulnerabilities/package_managers.py Outdated Show resolved Hide resolved
vulnerabilities/package_managers.py Show resolved Hide resolved
vulnerabilities/package_managers.py Outdated Show resolved Hide resolved
vulnerabilities/package_managers.py Show resolved Hide resolved
Signed-off-by: 司芳源 <[email protected]>
@sify21 sify21 requested a review from sbs2001 December 21, 2021 02:46
Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

Thanks!
Do you think you could add a few unit tests before we merge?

vulnerabilities/importers/github.py Outdated Show resolved Hide resolved
vulnerabilities/package_managers.py Outdated Show resolved Hide resolved
vulnerabilities/package_managers.py Outdated Show resolved Hide resolved
@pombredanne pombredanne added this to the v30.0 milestone Feb 2, 2022
sify21 and others added 3 commits February 7, 2022 11:14
Signed-off-by: 司芳源 <[email protected]>
@sify21 sify21 requested a review from pombredanne February 7, 2022 08:32
@sify21
Copy link
Contributor Author

sify21 commented Feb 7, 2022

Hi @pombredanne, I added some tests. I don't know how to resolve conflicts on github, should I pull the latest changes and resolve the conflicts on my fork?

@pombredanne
Copy link
Member

I added some tests. I don't know how to resolve conflicts on github, should I pull the latest changes and resolve the conflicts on my fork?

yes, you need to resolve these git conflicts locally, eventually rebasing and amending as needed and then force push to your branch once this is done

@sify21
Copy link
Contributor Author

sify21 commented Feb 7, 2022

@pombredanne merged

@pombredanne
Copy link
Member

Thanks! There is a merge commit that's making the DCO bot unhappy... I will ignore this

Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

Thank you for the update. Do you mind to run black to reformat your code?
make black should be enough

Signed-off-by: 司芳源 <[email protected]>
@sify21
Copy link
Contributor Author

sify21 commented Feb 8, 2022

@pombredanne reformatted

Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

@sify21 I have added a few extra comments for your consideration.

vulnerabilities/tests/test_package_managers.py Outdated Show resolved Hide resolved
vulnerabilities/package_managers.py Show resolved Hide resolved
vulnerabilities/package_managers.py Outdated Show resolved Hide resolved
vulnerabilities/package_managers.py Outdated Show resolved Hide resolved
vulnerabilities/package_managers.py Outdated Show resolved Hide resolved
vulnerabilities/package_managers.py Outdated Show resolved Hide resolved
Signed-off-by: 司芳源 <[email protected]>
Signed-off-by: 司芳源 <[email protected]>
Signed-off-by: 司芳源 <[email protected]>
@sify21 sify21 requested a review from pombredanne February 9, 2022 08:27
Signed-off-by: 司芳源 <[email protected]>
@Hritik14
Copy link
Collaborator

@sify21 As the development in this branch is going on and we wanted to move ahead with #476, rebasing/merging this with/from main will likely cause merge conflicts. Please accept the incoming changes for the import statements.
Alternatively, give kdiff3 a try for solving merge conflicts, it's really smart.

@sify21
Copy link
Contributor Author

sify21 commented Feb 14, 2022

@Hritik14 merged

Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

isort fails... I am running isort and merging now!
Thank you ++

@pombredanne pombredanne merged commit 6b105e6 into aboutcode-org:main Feb 15, 2022
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.

Collect go vulnerabilities from github api
4 participants