-
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
Dump importer_yielder in favor of IMPORTER_REGISTRY and drop Etags #600
Dump importer_yielder in favor of IMPORTER_REGISTRY and drop Etags #600
Conversation
457c91f
to
4a24a14
Compare
4a24a14
to
7175362
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.
Thanks! I have a few suggestions and ... Could you add tests?
89cdfee
to
058b9a8
Compare
IMPORTER_REGISTRY is neater and does not do any magical string -> object conversion. The registry looks more in sync with improvers. Fixes: aboutcode-org#501 Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Etags are meant for transient usage in browsers and are not meant for any long term usage. Fixes: aboutcode-org#321 Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
058b9a8
to
0c4905e
Compare
The name DataSource and Importer were majorly used interchangeably although they represent the same concept. Also, the importers now require a mandatory ``spdx_license_expression`` to run Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Same for improvers IMPROVER_REGISTRY -> IMPROVERS_REGISTRY Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
We can use @classmethod and @Property together in python 3.9. Not so in 3.8 Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
0c4905e
to
c62f81f
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.
Thanks... see a few final nits for your consideration.
|
||
# TODO: Needs rewrite |
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 an issue to track may work better?
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 will be a part of #597
|
||
|
||
def test_ImportRunner_existing_package_and_new_vulnerability(db): |
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.
Could this test be kept somehow? to valid that we never will allow twice the same set of imported data?
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.
assert vuln_refs[0].url == "https://example.com/with/more/info/CVE-2020-13371337" | ||
|
||
|
||
def test_ImportRunner_new_package_version_affected_by_existing_vulnerability(db): |
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.
IMHO this may be need to be kept
assert impacted_package.vulnerability.vulnerability_id == "CVE-2020-13371337" | ||
|
||
|
||
# def test_ImportRunner_fixed_package_version_is_added(db): |
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.
Could this test have been of value? and need to be kept?
# assert resolved_package.vulnerability.vulnerability_id == "CVE-2020-13371337" | ||
|
||
|
||
def test_ImportRunner_updated_vulnerability(db): |
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.
IMHO this may be need to be kept, moved to improvers?
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.
All of these will be moved to improvers. Importers cannot test relationships. It will be a part of a new PR. Tracked in: #612
Signed-off-by: Hritik Vijay <hritikxx8@gmail.com> Co-authored-by: Philippe Ombredanne <pombredanne@gmail.com>
61023e6
to
45cdaf5
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.
LGTM! 👍
The name
DataSource
andImporter
are majorly used interchangeably. This needs to be fixed in a different PR.