Split monolithic "vulnerabilities" Django app #132
Conversation
Signed-off-by: Haiko Schol <hs@haikoschol.com>
Signed-off-by: Haiko Schol <hs@haikoschol.com>
Signed-off-by: Haiko Schol <hs@haikoschol.com>
Signed-off-by: Haiko Schol <hs@haikoschol.com>
Signed-off-by: Haiko Schol <hs@haikoschol.com>
Signed-off-by: Haiko Schol <hs@haikoschol.com>
| @@ -26,7 +26,7 @@ | |||
|
|
|||
| from rest_framework.routers import DefaultRouter | |||
|
|
|||
| from vulnerabilities.api import PackageViewSet | |||
| from api.views import PackageViewSet | |||
There was a problem hiding this comment.
api feels like it is too generic a module name.... What could be more discriminant? IMHO we need a top level module
There was a problem hiding this comment.
Also could it make sense to have all the sources under an src directory (that's a tad less common for Django)?
I'm sure it's possible to move the entire Django project into a directory called src. There wouldn't be much left in the top-level dir of the repository, but maybe that will change over time.
If you want to have i.e. manage.py outside of a src directory that might require some more fiddling but should be doable as well.
What benefits do you see in having a src dir?
api feels like it is too generic a module name.... What could be more discriminant?
The scope of the name is all apps in the Django project vulnerablecode. As long as there is no ambiguity when talking about "the API of vulnerablecode" I think short, generic names are fine. It would be different if we wanted to make the api app reusable in other Django projects or turn vulnerablecode into a library.
Having said that, I don't insist on the name at all but I can't think of a better one right now either.
IMHO we need a top level module
To have it mentioned in import statements? Or what would be the benefit? I think we shouldn't stray too far from what Django expects in terms of project structure. I remember reading somewhere about a "flat structure" for Django projects without the individual apps a long time ago. However that sounds like it would be less nesting, not more.
| @@ -16,8 +16,8 @@ before_script: | |||
| - ./manage.py migrate | |||
|
|
|||
| script: | |||
| - python3.6 -m pytest -v vulnerabilities/tests/test_scrapers.py vulnerabilities/tests/test_api_data.py | |||
| - ./manage.py test vulnerabilities/tests | |||
| - python3.6 -m pytest -v importers/tests/test_scrapers.py api/tests/test_cve_search.py | |||
There was a problem hiding this comment.
Since you are changing this, what about running pytest -vvs only and adding a proper entry in setup.cfg to guide the discovery?
There was a problem hiding this comment.
I thought about changing this since I don't like passing files as arguments here either. But I don't know how to do this without reading some docs and I figured it will be changed as part of #127 anyway.
|
|
||
| def package_version(request, name, version): | ||
| """ | ||
| Queries the cve-search api with a package |
There was a problem hiding this comment.
Do we really want to call this cve-search?
This is a more generic vulnerability search than just CVE so I would refrain to use CVE as a term unless we are specifically dealing with the CVE... since many vulnerability may not have a CVE in the future.
There was a problem hiding this comment.
My question here would rather be do we really want to have this endpoint at all. It is a proxy to http://cve.circl.lu/api/search/, so the name is accurate but I'm not sure what the purpose is.
I just kept this mostly as is (modulo some file renaming) to keep the scope of the PR small.
| @@ -58,23 +58,23 @@ class Migration(migrations.Migration): | |||
| ('platform', models.CharField(blank=True, help_text='Platform eg:maven', max_length=50)), | |||
| ('name', models.CharField(blank=True, help_text='Package reference name eg:org.apache.commons.io', max_length=50)), | |||
| ('version', models.CharField(blank=True, help_text='Reference version', max_length=50)), | |||
| ('package', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='vulnerabilities.Package')), | |||
| ('package', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='core.Package')), | |||
There was a problem hiding this comment.
Sam comment as for api: core is too generic IMHO
There was a problem hiding this comment.
Should we keep the name vulnerabilities for the app containing the models instead? It's annoying to type but I can live with that. :)
|
Closed in favour of #147 |
This change splits the single Django app used so far into three separate ones:
core(for DB models and migrations)importers(for code that retrieves data from package registries, etc.)API(for the HTTP API of vulnerablecode)Furthermore it consolidates some files, slightly improves naming and uses database fixtures for the API tests. The latter avoids having the API test call code from the
importersapp. This reduces the blast radius of breaking changes and eases onboarding for new contributers.closes #122