Skip to content
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

Fixes #10782 - global host status #2580

Closed
wants to merge 1 commit into from

Conversation

ares
Copy link
Member

@ares ares commented Jul 28, 2015

Keeping my commits separately for easier reviewing. There are two things I didn't refactor in this PR, if we agree it's a good direction I'll create redmine issues.

app/models/report.rb line 66
it would be very nice to refactor this behaviour and ensure the status setter always gets integer number from the importer

app/services/report_status_calculator.rb line 56
replace dynamic define_method and use something like status_of('applied') everywhere instead (I don't think this is highly important)

@domcleal
Copy link
Contributor

Could you fix the commit messages, so as not to fight the system? Thanks. I'll also reset the review label as I don't believe it's been reviewed yet.

@ares
Copy link
Member Author

ares commented Jul 30, 2015

I fixed messages. I removed Not yet reviewed since you did the same previously so I thought you might have seen it already and hadn't found anything ( 😆 )

@domcleal
Copy link
Contributor

Hah. No, probably a misclick.

@@ -0,0 +1,32 @@
class CreateHostStatus < ActiveRecord::Migration
def self.up
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No "self." on these methods any more.

@ares
Copy link
Member Author

ares commented Sep 3, 2015

Removed a.txt (sorry I needed to inject some commit during squashing and forgot to remove it) and introduced a new relation to configuration_status which is now used in scoped search queries.

@ares ares force-pushed the fix/10782_reports branch 2 times, most recently from 1a8229d to df74227 Compare September 4, 2015 07:17
@domcleal
Copy link
Contributor

domcleal commented Sep 4, 2015

The tests are red, a page is failing with internal server errors.

@ares
Copy link
Member Author

ares commented Sep 4, 2015

@domcleal I'm working on that, I hoped you won't notice before I fix it (rebase issue after stabby lamda change...)

@ares
Copy link
Member Author

ares commented Sep 4, 2015

🍏


module ClassMethods
def scoped_search_status(status, options)
options.merge({ :offset => Report::METRIC.index(status.to_s), :word_size => Report::BIT_NUM })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge! else it doesn't work at all.

Introduce new global host status that is composed of host substatuses.
Each substatus defines a mapping to the global one which can result in
three values
* OK
* WARN
* ERROR

Plugins can add their own substatuses. These are automatically
propagated also to API.

Thanks to Tomas Strachota who wrote the original code.
@domcleal
Copy link
Contributor

domcleal commented Sep 4, 2015

Thanks @ares and @tstrachota, merged as e54016d.

Could you add a section to http://projects.theforeman.org/projects/foreman/wiki/How_to_Create_a_Plugin about how to create your own substatus? This could be useful for a variety of plugins.

@domcleal domcleal closed this Sep 4, 2015
@ares ares deleted the fix/10782_reports branch September 4, 2015 12:47
@ares
Copy link
Member Author

ares commented Sep 4, 2015

For the reference, the section was added here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants