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

migrate current alpine importer to alpine importer-improver model #623

Merged
merged 2 commits into from
Mar 4, 2022
Merged

migrate current alpine importer to alpine importer-improver model #623

merged 2 commits into from
Mar 4, 2022

Conversation

TG1999
Copy link
Contributor

@TG1999 TG1999 commented Feb 11, 2022

Signed-off-by: Tushar Goel [email protected]

@TG1999 TG1999 changed the title migrate current alpine importer to alpine importer-improver model migrate current alpine importer to alpine importer-improver model #620 Feb 11, 2022
@TG1999 TG1999 changed the title migrate current alpine importer to alpine importer-improver model #620 migrate current alpine importer to alpine importer-improver model Feb 11, 2022
@TG1999
Copy link
Contributor Author

TG1999 commented Feb 11, 2022

for issue #620

@Hritik14
Copy link
Collaborator

@TG1999 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.

@TG1999
Copy link
Contributor Author

TG1999 commented Feb 14, 2022

@Hritik14 Resolved merge conflicts

@TG1999 TG1999 force-pushed the migration/alpine_linux branch 2 times, most recently from b68f076 to 3360eb3 Compare February 17, 2022 12:40
@TG1999
Copy link
Contributor Author

TG1999 commented Feb 17, 2022

also solves issue #628 and #629

@TG1999 TG1999 force-pushed the migration/alpine_linux branch 2 times, most recently from 50ba90a to 5ff44fe Compare February 17, 2022 12:56
@Hritik14 Hritik14 mentioned this pull request Feb 17, 2022
Closed
@TG1999
Copy link
Contributor Author

TG1999 commented Feb 17, 2022

@pombredanne @Hritik14 @sbs2001 Please check my current approach for the importer, if this looks good I will proceed with writing tests for this

pombredanne
pombredanne requested changes Feb 18, 2022
View reviewed changes
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.

Here are a few nits for your review!

vulnerabilities/importer.py Outdated Show resolved Hide resolved
vulnerabilities/importer.py Outdated Show resolved Hide resolved
vulnerabilities/importer.py Outdated Show resolved Hide resolved
vulnerabilities/importer.py Outdated Show resolved Hide resolved
vulnerabilities/importer.py Outdated
fixed_version: Optional[Version] = None

def __post_init__(self):
if self.package.version:
if self.package.version or not (self.affected_version_range or self.fixed_version):
raise ValueError
Copy link
Member

Choose a reason for hiding this comment

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

Please add a message

vulnerabilities/importers/alpine_linux.py Outdated
return advisories
# TODO: Handle the CVE-????-????? case
yield AdvisoryData(
summary="",
Copy link
Member

Choose a reason for hiding this comment

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

Why an empty summary? Is is mandatory?

vulnerabilities/importers/alpine_linux.py Outdated
),
fixed_version=AlpineLinuxVersion(version),
)
for arch in archs
Copy link
Member

Choose a reason for hiding this comment

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

What if here is no archs?

vulnerabilities/importers/alpine_linux.py Outdated
yield AdvisoryData(
summary="",
references=references,
affected_packages=[
Copy link
Member

Choose a reason for hiding this comment

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

Create this before in a separate easier to read loop

vulnerabilities/importers/alpine_linux.py Outdated
)
for arch in archs
],
aliases=[vuln_ids[0] if is_cve(vuln_ids[0]) else ""],
Copy link
Member

Choose a reason for hiding this comment

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

Do not return a list of aliases with an empty string in it.

vulnerabilities/improvers/default.py Outdated
@@ -38,7 +39,7 @@ def get_inferences(self, advisory_data: AdvisoryData) -> Iterable[Inference]:
)


def get_exact_purls(affected_package: AffectedPackage) -> (List[PackageURL], PackageURL):
def get_exact_purls(affected_package: AffectedPackage) -> Tuple[List[PackageURL], PackageURL]:
Copy link
Member

Choose a reason for hiding this comment

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

Move these changes in a separate PR.

pombredanne
pombredanne requested changes Feb 25, 2022
View reviewed changes
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. LGTM... just a few cosmetic nits for your consideration. Could you also add some tests?

vulnerabilities/importer.py Outdated Show resolved Hide resolved
vulnerabilities/importers/alpine_linux.py Outdated Show resolved Hide resolved
vulnerabilities/importers/alpine_linux.py Outdated Show resolved Hide resolved
vulnerabilities/importers/alpine_linux.py Outdated Show resolved Hide resolved
vulnerabilities/importers/alpine_linux.py Outdated Show resolved Hide resolved
vulnerabilities/importers/alpine_linux.py Outdated Show resolved Hide resolved
vulnerabilities/importers/alpine_linux.py Outdated Show resolved Hide resolved
vulnerabilities/importers/alpine_linux.py Outdated Show resolved Hide resolved
vulnerabilities/importers/alpine_linux.py Show resolved Hide resolved
vulnerabilities/improvers/default.py Outdated Show resolved Hide resolved
@TG1999 TG1999 force-pushed the migration/alpine_linux branch 2 times, most recently from 7bdea4c to 4d9b13a Compare March 1, 2022 16:57
@Hritik14 Hritik14 linked an issue Mar 1, 2022 that may be closed by this pull request
Add license_url in Advisory Data #634
Closed
Hritik14
Hritik14 requested changes Mar 1, 2022
View reviewed changes
Copy link
Collaborator

@Hritik14 Hritik14 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! I've marked a few things that I'd like you to consider.
Also, In general, there are a lot of asserts. IMO a loud logging mechanism should serve for those asserts than failing entirely and much of the asserts should make their place in the tests.

vulnerabilities/importers/alpine_linux.py Outdated Show resolved Hide resolved
vulnerabilities/importer.py Outdated Show resolved Hide resolved
vulnerabilities/importer.py Outdated Show resolved Hide resolved
vulnerabilities/importers/alpine_linux.py Show resolved Hide resolved
vulnerabilities/importers/alpine_linux.py Outdated Show resolved Hide resolved
vulnerabilities/references.py Outdated Show resolved Hide resolved
vulnerabilities/references.py Outdated Show resolved Hide resolved
vulnerabilities/references.py Outdated Show resolved Hide resolved
vulnerabilities/references.py Outdated Show resolved Hide resolved
vulnerabilities/references.py Outdated Show resolved Hide resolved
TG1999
TG1999 commented Mar 3, 2022
View reviewed changes
vulnerabilities/improver.py Outdated Show resolved Hide resolved
TG1999
TG1999 commented Mar 3, 2022
View reviewed changes
vulnerabilities/improver.py Outdated Show resolved Hide resolved
@TG1999
Migrate alpine importer to importer improver model
a69c886 
Current alpine importer models need to be refactored to give AdvisoryData instead of Advisory, also add some validation to parse license expression and make affected_version_range optional

Add tests to test scraping of webpages and parsing of data

Signed-off-by: Tushar Goel <[email protected]>
@TG1999
fix nix
104c760 
Signed-off-by: Tushar Goel <[email protected]>
@TG1999 TG1999 merged commit 8c69661 into aboutcode-org:main Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hritik14 Hritik14 Hritik14 requested changes

@sbs2001 sbs2001 Awaiting requested review from sbs2001

@pombredanne pombredanne Awaiting requested review from pombredanne

Assignees
No one assigned
Labels
import-improver-migration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add license_url in Advisory Data
3 participants
@TG1999 @Hritik14 @pombredanne