-
Notifications
You must be signed in to change notification settings - Fork 198
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
Write packages and vulnerabilities at the time of import #1280
Write packages and vulnerabilities at the time of import #1280
Conversation
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.
Some nits for your consideration.
vulnerabilities/improve_runner.py
Outdated
@@ -36,11 +37,18 @@ class ImproveRunner: | |||
improver and parsing the returned Inferences into proper database fields | |||
""" | |||
|
|||
def __init__(self, improver_class): | |||
self.improver_class = improver_class | |||
def __init__(self, improver_class=None, improver: Improver = None): |
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.
May be keeping the class design and passing kwargs can work too?
def __init__(self, improver_class=None, improver: Improver = None): | |
def __init__(self, improver_class, **kwargs): | |
self.improver = improver_class(**kwargs) |
vulnerabilities/improve_runner.py
Outdated
f"Inconsistent summary for {vulnerability!r}. " | ||
f"Existing: {vulnerability.summary}, provided: {summary}" | ||
) | ||
summary = summary.strip() |
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.
This may be best in another PR
@DennisClark we need your input on this, we use advisories as a screenshot of the status of the advisory at a given point of time. A single advisory goes through many improver processes to draw inferences, what should be the best possible way to get a status on advisory model to know which improvers have already ran through a particular advisory ? |
@TG1999 Here are a few thoughts about advisories:
I don't think we necessarily need a "status" on the advisory, since the list of improver_name:improver_date values ought to give us what would be really useful here (what has been run and when). |
@DennisClark thanks for these excellent suggestions. |
0fc89ac
to
6b4dc69
Compare
e92ab1f
to
8234a49
Compare
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.
Here are some nits for your consideration
vulnerabilities/migrations/0040_remove_advisory_date_improved_advisory_date_imported.py
Show resolved
Hide resolved
8234a49
to
1545ea0
Compare
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.
Here are a few nits... Either apply now or later!
LGTM otherwise!
vulnerabilities/import_runner.py
Outdated
logger.info(f"Finished import for {importer_name}. Imported {count} advisories.") | ||
|
||
def do_import(self, advisories) -> None: | ||
advisory_importer = AdvisoryBasedDefaultImprover(advisories=advisories) |
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.
We are now using importer ... why still keeping the improver around in names of variables and objects?
vulnerabilities/import_runner.py
Outdated
) | ||
except Exception as e: | ||
logger.info(f"Failed to process advisory: {advisory!r} with error {e!r}") | ||
logger.info("Finished improving using %s.", advisory_importer.__class__.qualified_name) |
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.
logger.info("Finished improving using %s.", advisory_importer.__class__.qualified_name) | |
logger.info("Finished importing using %s.", advisory_importer.__class__.qualified_name) |
Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
d74d9fa
to
0ef58f2
Compare
public instance of VCIO contains more than 35 million advisories ( the volume is so high, due to minor changes in every advisory maybe sometime or the changes in models), which takes a lot of time. If we improve the advisories just after the import this will drop volume hugely.