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 #31213 - hostgroup API facets extension point #8115

Merged
merged 3 commits into from
Jan 6, 2021

Conversation

ezr-ondrej
Copy link
Member

@ezr-ondrej ezr-ondrej commented Oct 29, 2020

Adds Facet extension points for Hostgroup API responses.
Adds forgotten refresh for existing entires on hostgroup.

EDIT: This is needed by foreman_puppet_enc plugin since theforeman/foreman_puppet@40cfb9b#diff-8dc4b7b4116cb63c7a7b1b3c40a25c515f61ccceff63bb4b387eda1f10607988R102-R104

@theforeman-bot
Copy link
Member

Issues: #31213

Copy link
Member

@ShimShtein ShimShtein left a comment

Choose a reason for hiding this comment

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

Do we have the usage counterpart for this?

@ezr-ondrej
Copy link
Member Author

Do we have the usage counterpart for this?

Sorry not to include it right away, this is the counterpart:
theforeman/foreman_puppet@40cfb9b#diff-8dc4b7b4116cb63c7a7b1b3c40a25c515f61ccceff63bb4b387eda1f10607988R102-R104
Just clone the plugin, require it :)

Copy link
Member

@ShimShtein ShimShtein left a comment

Choose a reason for hiding this comment

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

Unfortunately, katello test failures are related.

Log from local run:

2020-11-22T10:42:54 [I|app|] Backtrace for 'Action failed' error (ActionView::Template::Error): undefined method `content_view_version' for #<Katello::Hostgroup::ContentFace
t:0x0000000018f6fd70>
 | /home/vagrant/foreman/.vendor/ruby/2.5.0/gems/activemodel-6.0.3.4/lib/active_model/attribute_methods.rb:432:in `method_missing'
 | /home/vagrant/foreman/.vendor/ruby/2.5.0/gems/audited-4.9.0/lib/audited/auditor.rb:99:in `method_missing'
 | /home/vagrant/katello/app/views/katello/api/v2/content_facet/show.json.rabl:6:in `block (2 levels) in cached_source_3397887158937366876'
 | /home/vagrant/foreman/.vendor/ruby/2.5.0/gems/rabl-0.14.3/lib/rabl/builder.rb:157:in `node'
 | /home/vagrant/foreman/.vendor/ruby/2.5.0/gems/rabl-0.14.3/lib/rabl/builder.rb:116:in `block in compile_settings'
...
 | /home/vagrant/foreman/.vendor/ruby/2.5.0/gems/rabl-0.14.3/lib/rabl/engine.rb:287:in `partial'
 | /home/vagrant/foreman/app/views/api/v2/hostgroups/show.json.rabl:25:in `block (2 levels) in cached_source_1252319207728911631'
 | /home/vagrant/foreman/.vendor/ruby/2.5.0/gems/rabl-0.14.3/lib/rabl/builder.rb:157:in `node'
 | /home/vagrant/foreman/.vendor/ruby/2.5.0/gems/rabl-0.14.3/lib/rabl/builder.rb:116:in `block in compile_settings'
...
 | /home/vagrant/foreman/.vendor/ruby/2.5.0/gems/rabl-0.14.3/lib/rabl/engine.rb:49:in `render'
 | /home/vagrant/foreman/config/initializers/rabl_init.rb:49:in `render'
 | /home/vagrant/foreman/app/views/api/v2/hostgroups/update.json.rabl:3:in `_40ee894962f6d73f981bdbae3abcc910'

@ezr-ondrej
Copy link
Member Author

Unfortunately, katello test failures are related.

I think it's because of initialization of the association being done twice. Rails are not handling it that well.
I've rewritten it to register the associations only once, I think it's bit better like this, let me know what you think.
Maybe the method naming is bit off? 🤔

@@ -41,16 +41,20 @@ def refresh_facet_relations
# host.foo # => will call Host.my_facet.foo
def register_facet_relation(facet_config)
Copy link
Member Author

Choose a reason for hiding this comment

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

this method is now not being called IIRC, but not sure about that.

@ezr-ondrej
Copy link
Member Author

I've extracted the refresh / register to #8151

@ezr-ondrej ezr-ondrej force-pushed the add_hostgroup_api_facets branch 2 times, most recently from 05bfb01 to 2b5a18c Compare November 23, 2020 13:10
Copy link
Member

@ShimShtein ShimShtein left a comment

Choose a reason for hiding this comment

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

Still failing on katello content hosts page:
image

15:36:05 rails.1   | 2020-11-23T15:36:05 [W|app|da21f3e1] Action failed
15:36:05 rails.1   | 2020-11-23T15:36:05 [I|app|da21f3e1] Backtrace for 'Action failed' error (ActionView::Template::Error): undefined local variable or method `hostgroup' for #<Rabl::Engine:0x0000000014698b10>
15:36:05 rails.1   |  da21f3e1 | /home/vagrant/foreman/.vendor/ruby/2.5.0/gems/rabl-0.14.3/lib/rabl/engine.rb:359:in `method_missing'
15:36:05 rails.1   |  da21f3e1 | /home/vagrant/foreman/app/views/api/v2/hosts/main.json.rabl:61:in `block in cached_source_3230678856672812020'
15:36:05 rails.1   |  da21f3e1 | /home/vagrant/foreman/app/views/api/v2/hosts/main.json.rabl:59:in `each'
15:36:05 rails.1   |  da21f3e1 | /home/vagrant/foreman/app/views/api/v2/hosts/main.json.rabl:59:in `cached_source_3230678856672812020'
...
15:36:05 rails.1   |  da21f3e1 | /home/vagrant/foreman/config/initializers/rabl_init.rb:49:in `render'
15:36:05 rails.1   |  da21f3e1 | /home/vagrant/foreman/app/views/api/v2/hosts/index.json.rabl:3:in `_d400f40cad14669013188f1e28fe919e'

@ezr-ondrej
Copy link
Member Author

[test katello]

1 similar comment
@ezr-ondrej
Copy link
Member Author

[test katello]

Adds Facet extension points for Hostgroup API responses.
Refactor refresh for facets, to ensure all run just once.

Now the already registered entries are being run once the facet entry point is configured.
The hook is registered also on the entry point registration.
It should give us more clear API of the new facet entry poin registration.
fixes an issue with an Model having access to general facet definitions (entries)
rather than it's own (model based entries) definitions.
@ezr-ondrej
Copy link
Member Author

[test katello] https://ci.theforeman.org/job/test_develop_pr_katello/6695 passed, it could be a node host that makes the difference or the test concurrency

@ezr-ondrej
Copy link
Member Author

I've added tests and we are 🍏

@tbrisker
Copy link
Member

tbrisker commented Jan 5, 2021

ping @ShimShtein what's the status here?

@ShimShtein ShimShtein merged commit 477d457 into theforeman:develop Jan 6, 2021
@ShimShtein
Copy link
Member

Merged, thanks @ezr-ondrej!

@ezr-ondrej ezr-ondrej deleted the add_hostgroup_api_facets branch January 6, 2021 13:29
stefanlasiewski pushed a commit to stefanlasiewski/foreman that referenced this pull request Jan 20, 2021
* Fixes #31213 - hostgroup API facets extension point

Adds Facet extension points for Hostgroup API responses.
Refactor refresh for facets, to ensure all run just once.

Now the already registered entries are being run once the facet entry point is configured.
The hook is registered also on the entry point registration.
It should give us more clear API of the new facet entry poin registration.

* Refs #31213 - model specific definition iterrable

fixes an issue with an Model having access to general facet definitions (entries)
rather than it's own (model based entries) definitions.

* Refs #31213 - Add tests for API response with facets
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants