search

trustification / trustify

Bringing together SBOMs and advisories.
Apache License 2.0
10 stars 20 forks source link

Relocate `get_*s` to actual services in `fundamental` #547

Open ctron opened 4 months ago

ctron commented 4 months ago

From here: https://github.com/trustification/trustify/pull/532#discussion_r1677807747

related: I'd still argue we should be deleting get_plurals() and defer that ish to the appropriate fundamental Service.

ctron commented 4 months ago

@bobmcwhirter @jcrossley3 Ok, the downside of this is, that we'd need to move all (or most) tests from the ingestor module to either the fundamental module or revive the integration-tests.

jcrossley3 commented 4 months ago

Sorry, I don't understand why the move is required... or even what get_plurals() is.

ctron commented 4 months ago

I think functions like get_vulnerabiltities … It could also cover get_vulnerability. However, those functions might be used in our pattern of ingest_foo returning get_foo results when those exists.

ctron commented 4 months ago

Sorry, I don't understand why the move is required... or even what get_plurals() is.

The move is required because tests in modules/ingestor make use of functions like get_vulnerabilities.

jcrossley3 commented 4 months ago

Ok, well if we feel strongly that those fn's should be moved to fundamental, then their integs should follow. I'm -1 on reviving the integration-tests/ module.

bobmcwhirter commented 4 months ago

Yah, the downside of removing get_* is that it makes it difficult to introspect ingestion results in a unit-test-like area.

Putting the tests in fundamental would work, but would also be somewhat obliquely testing ingestor.

Which is fine?

jcrossley3 commented 4 months ago

It's entirely possible our tests are ill-scoped, maybe some of our integs are better organized as units beneath ingestor/?

jcrossley3 commented 4 months ago

Yah, the downside of removing get_* is that it makes it difficult to introspect ingestion results in a unit-test-like area.

Coming from the school of "if it's hard to test, it's probably wrong", maybe it's correct that the Vulnerability service delegates to the Ingestor for its needed data?

bobmcwhirter commented 4 months ago

That's kinda where we've been heading with ingest_documents(...) but it's the inverse really... how can we test the ingestor directly?

gildub commented 3 months ago

If fundamental is a layer on top of REST API and graphql, which doesn't use REST API requests although it could (which is the approach used when that's the only API available), gets helpers from lower layers, including existing functions already available in ingestor. Making those functions available to other modules by providing a common layer makes sense.