Adding scan_results table for ScanResult model

[enoodle]

To support reporting and displaying the last OpenSCAP scan result for container images, as requested in ManageIQ/manageiq-providers-kubernetes#100

[Fryguy]

Migration looks good to me, but I believe this needs an update to the db/schema.yml

Will wait on approval from @simon3z with respect to ManageIQ/manageiq-providers-kubernetes#100 before merging.

[simon3z]

Will wait on approval from @simon3z with respect to ManageIQ/manageiq-providers-kubernetes#100 before merging.

@Fryguy @enoodle yeah before merging we definitely have to finalize ManageIQ/manageiq-providers-kubernetes#100
Thanks.

[enoodle]

@moolitayer @simon3z It seems that we got to a decision in
ManageIQ/manageiq-providers-kubernetes#100 So you should ack this PR as well.

[moolitayer]

LGTM 👍

[simon3z]

@enoodle @roliveri are we sure we want to keep these per object?

Should we have instead a new table with a polymorphic association so that we can keep in a single place regardless to what object they refer to? (VMs, Images, Container Images, etc.).

[roliveri]

@simon3z As we discussed, I think you're right. We can have a scan_status table used to store the scan status and last scan time. I would also suggest it should contain a scan_type field, so we can differentiate between different types of scans going forward (standard vs oscap for example).

[enoodle]

@simon3z @roliveri @moolitayer PTAL I have added the new table last_scan_results

[simon3z]

@miq-bot assign @Fryguy

@roliveri can you explicitly ack this if it looks good to you?

[roliveri]

We may want to add a scan_type field, so we can differentiate when we have multiple types of scans for the same resource (OSCAP and legacy for example), that can be preformed independently.

[moolitayer]

I would link this to the job that created the status. We could get the target resource through that. Is that enough to have the needed info of scan_type @roliveri ?
(I know that for ContainerManager::Scanning::Job we always have OpenSCAP AND package analysis)

[roliveri]

I don't think so. Assuming the scan type based on the resource won't be enough. As I mentioned, going forward, different scans may be performed independently for the same resource, so they may be performed by different jobs. Even if we could obtain the scan type through the job, do job objects persist after the jobs have completed? If not, the link to the job could go stale while the scan results are still linked to the resource.

[moolitayer]

Can we have different scan types in a vm scanning job? Anyway regardless the new table might be a better place to keep the scan_type (and anything related to scan and not directly to job)

I still think the a link to the job would serve us (for example if we want to show last job info in container image screens). I don't see a purge or something that would delete job records but I might be missing something.

[roliveri]

We haven't had different scan types until now, so there's no concept of type in the scan job. However, I think we may need to rely on subclasses of Job more than we do now. The Job subclass could not only indicate the resource being scanned, but the type of scan as well. If the scanning process is significantly different for some types, it makes sense for them to have their own Job subclass.

I'm not saying we shouldn't link to Job. Only that we should make sure it will be there when we need it.

[enoodle]

I updated this with scan_type field. @moolitayer @roliveri I think we should keep it simple in this patch and link to the Job in a different set of PRs.

[roliveri]

Looks good to me.
Once the tests are green, should be ok to merge.

[enoodle]

So apparently the tables in db/schema.yaml have to be ordered alphabetically or you get this unhelpful error:

       vmdb_test is not structured as expected.
       Schema validation failed for host localhost:
     
       Expected schema table order does not match sorted current tables.
       Use 'rake db:write_schema' to generate the new expected schema when making changes.
     
       Refer to http://talk.manageiq.org/t/new-schema-specs-for-new-replication/1404 for detail
     # ./plugins/manageiq-schema/spec/replication/util/schema_structure_spec.rb:13:in `block (2 levels) in <top (required)>'

and rake doesn't really know how to build this task so it won't work to fix it.
Super annoying.

I ordered the table now, hopefully this will make the test green.

[miq-bot]

Checked commit enoodle@ec53a4c with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

[Fryguy]

Use 'rake db:write_schema' to generate the new expected schema when making changes.

Does rake db:write_schema not work for you?

[enoodle]

@Fryguy No, Maybe I am not running it well. I tried from the main ManageIQ directory but it couldn't build the task. From the manageiq-schema plugin directory I got ActiveRecord::NoDatabaseError: FATAL: database "dummy_development" does not exist. linking "spec/manageiq" temporarily didn't help either.

[Fryguy]

You need a fully raked up database to do it. If you've done rake spec:setup, then you can use test database with:

RAILS_ENV=test bundle exec rake db:write_schema

[enoodle]

@Fryguy Thanks, this was working for me now.
This patch is green now, can we merge it?