Add usage report command

[ares]

This is the PoC of generating yaml representing the usage of the instance. Test wit

foreman-maintain report generate

Currently, the command searches for all ReportChecks, executes them and collects the data from them. Then it prints out the resulting YAML to stdout.

[evgeni]

does that take all reports, their .data and merge those hashes? what if two reports accidentally define the same hash key?

[ares]

Yes. I think that's very unlikely as the keys are very descriptive. Is it worth printing a warning to STDERR? Or perhaps better, having a some sort of a test that check for all keys in all checks? That may be a bit harder though as some of them are generated dynamically. Perhaps each check could have some explicit prefix that could that be verified, but that's likely schema v2 thing.

[evgeni]

What if we add an add_data(key, value) method to Report, and it does something like

data[self.class.name+'_'+key] = value

I wouldn't block on this, but I think it's a potential source for hard to debug issues once a "not so descriptive" name leaks into the set.

[ares]

@adamruzicka @ShimShtein thoughts?

[evgeni]

It would also save the transform_keys(to_s) here, which makes the code more readable?

[adamruzicka]

I'd leave this for v2. While we're at it, we should maybe shuffle things around a bit, the report classes are currently named somewhat arbitrarily

[ehelms]

If you keep something big like this I would find it more readable to split it across lines:

          report_data = scenario.
            steps.
            map(&:data).
            compact.
            reduce(&:merge).
            transform_keys(&:to_s)

[evgeni]

I think Report should have a query itself, no need to call feature() all the time

[ares]

nope, undefined method #query

[evgeni]

I said "should", not "does" :)
As in: we should implement a Report.query and use that, instead of calling feature(:db).query all the time ;)

[adamruzicka]

ares#15

[evgeni]

@ares did you plan to address this, given Adam has a patch for it?

[ares]

I address most of the comments, rebased the PR and added one more report definition around RBAC

[evgeni]

Can we get some basic tests for the new Report class you're introducing and the new scenario you're adding?
Tests for the individual reports are probably overkill (and would result in a lot of mocking, so doubtfully add any value)

[ehelms]

Break this method up instead of disabling rubocop. This could also help with the comments listed in these files. You can use ruby to help, e.g.

def number_of_users
def number_of_non_admin_users
def number_of_custom_roles

[ehelms]

Fun fact I learned and stuck with me -- https://kirillshevch.medium.com/misusing-of-notimplementederror-in-ruby-531dd866f461

Yes this code base already does it, but sharing is caring.

[ares]

All comments should be addressed atm, could you please re-review/merge?

[evgeni]

Are the remaining open points (method length in rbac.rb, NotImplementedError in report.rb, Report#query method and tests) planned to be addressed?

[adamruzicka]

Are the remaining open points (method length in rbac.rb, NotImplementedError in report.rb, Report#query method and tests) planned to be addressed?

I filed #976 to track all of those. Could we accept this PR as is and resolve those in additional PRs so that we can save the additional round-trip through @ares's fork?

[evgeni]

I'm fine with this a a first round as suggested by Adam

[evgeni]

this returns an empty string and not 0 if there are no custom roles

[adamruzicka]

Apparently, the query we generate isn't valid if there are no custom roles. Good catch

> query
=> " SELECT COUNT(*) AS COUNT FROM cached_user_roles WHERE cached_user_roles.role_id IN ()"

> feature(:foreman_database).query(query)
CSV::MalformedCSVError: Illegal quoting in line 1.

[evgeni]

Yeah, IN () is not valid:

foreman=# SELECT COUNT(*) AS COUNT FROM cached_user_roles WHERE cached_user_roles.role_id IN ();
ERROR:  syntax error at or near ")"
LINE 1: ...ROM cached_user_roles WHERE cached_user_roles.role_id IN ();
                                                                     ^

[evgeni]

some settings (like db_pending_seed, instance_id and bcrypt_cost) are always modified -- is it worth logging them here or should they be filtered?

[evgeni]

This produces quite a lot of output, which you probably don't want in the case where you run this via cron (as otherwise you'll get a mail from cron on every invocation. You could add --cron or --silent to suppress these?