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 #25097,#25277 - Associate reports & classes with taxonomy #6306

Merged
merged 1 commit into from Dec 11, 2018

Conversation

Projects
None yet
4 participants
@bastilian
Copy link
Member

bastilian commented Dec 5, 2018

This is an alternative to #6106

@theforeman-bot

This comment has been minimized.

Copy link
Member

theforeman-bot commented Dec 5, 2018

Issues: #25097 #25277

@ares

This comment has been minimized.

Copy link
Member

ares commented Dec 5, 2018

so this adds relations, do we also need to include Taxonomix or add some default scope so that resources are properly scoped?

@bastilian

This comment has been minimized.

Copy link
Member Author

bastilian commented Dec 5, 2018

@ares I'm not sure if I understand correctly what you mean. The API is taking care of building the query and scoping. When it get's a organization_id or location_id it will set these in parent_scope as scopes.

@tbrisker
Copy link
Member

tbrisker left a comment

I have to say, this looks much simpler than the other PR, if this fixes the issue I would rather go with this path. a couple of comments inline

@@ -28,6 +28,10 @@ class Taxonomy < ApplicationRecord
has_many :subnets, :through => :taxable_taxonomies, :source => :taxable, :source_type => 'Subnet'
has_many :auth_sources, :through => :taxable_taxonomies, :source => :taxable, :source_type => 'AuthSource'

has_many :puppetclasses, :through => :environments

This comment has been minimized.

Copy link
@tbrisker

tbrisker Dec 5, 2018

Member

Should this also have the other side on the puppetclass side?

This comment has been minimized.

Copy link
@bastilian

bastilian Dec 5, 2018

Author Member

The associations were not necessary in the case of puppetclasses to fix the API, but they are added now.

@@ -28,6 +28,10 @@ class Taxonomy < ApplicationRecord
has_many :subnets, :through => :taxable_taxonomies, :source => :taxable, :source_type => 'Subnet'
has_many :auth_sources, :through => :taxable_taxonomies, :source => :taxable, :source_type => 'AuthSource'

has_many :puppetclasses, :through => :environments
has_many :hosts, :through => :environments

This comment has been minimized.

Copy link
@tbrisker

tbrisker Dec 5, 2018

Member

taxonomies already have many hosts directly, they don't need to be defined through environments

This comment has been minimized.

Copy link
@bastilian

bastilian Dec 5, 2018

Author Member

It's doesn't seem to be able to find the hosts association properly though, without this association Organization.find(ID).reports fails:

[24] pry(main)> Organization.first.reports
2018-12-05T09:46:06 [D|sql|]   Organization Load (0.7ms)  SELECT  "taxonomies".* FROM "taxonomies" WHERE "taxonomies"."type" IN ('Organization') ORDER BY "taxonomies"."title" ASC LIMIT $1  [["LIMIT", 1]]
ActiveRecord::HasManyThroughAssociationNotFoundError: Could not find the association :hosts in model Taxonomy
from /home/bastilian/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/gems/activerecord-5.2.1/lib/active_record/reflection.rb:912:in `check_validity!'

This comment has been minimized.

Copy link
@tbrisker

tbrisker Dec 5, 2018

Member

hmm, the association is defined on the child classes, so maybe the reports association should also be defined there?

This comment has been minimized.

Copy link
@bastilian

bastilian Dec 5, 2018

Author Member

Yes, that works. Though, should we maybe think about moving the has_many_hosts one level up, or is it in the child classes for a specific reason?

This comment has been minimized.

Copy link
@tbrisker

tbrisker Dec 5, 2018

Member

I think it's there so the reverse is calculated correctly (i.e. on host there is both organization_id and location_id, not taxonomy_id)

@tbrisker
Copy link
Member

tbrisker left a comment

Looking good! can you just add the relevant tests from the other pr here so we ensure this works please?

@bastilian bastilian force-pushed the bastilian:fix/25097-2 branch from 9a87a6d to 13c78d6 Dec 6, 2018

@bastilian

This comment has been minimized.

Copy link
Member Author

bastilian commented Dec 6, 2018

@tbrisker The API controller tests are now in here as well. The puppetclasses test is passing, but config reports is missing something.

Organization.find(TEST_ORG_ID).config_reports returns the right result, but the API is building a query that misses the config report. This is the query built by the API:

"SELECT  \"reports\".\"id\" AS t0_r0, \"reports\".\"host_id\" AS t0_r1, \"reports\".\"reported_at\" AS t0_r2, \"reports\".\"created_at\" AS t0_r3, \"reports\".\"updated_at\" AS t0_r4, \"reports\".\"status\" AS t0_r5, \"reports\".\"metrics\" AS t0_r6, \"reports\".\"type\" AS t0_r7, \"reports\".\"origin\" AS t0_r8, \"taxonomies\".\"id\" AS t1_r0, \"taxonomies\".\"name\" AS t1_r1, \"taxonomies\".\"type\" AS t1_r2, \"taxonomies\".\"created_at\" AS t1_r3, \"taxonomies\".\"updated_at\" AS t1_r4, \"taxonomies\".\"ignore_types\" AS t1_r5, \"taxonomies\".\"description\" AS t1_r6, \"taxonomies\".\"label\" AS t1_r7, \"taxonomies\".\"ancestry\" AS t1_r8, \"taxonomies\".\"title\" AS t1_r9, \"taxonomies\".\"manifest_refreshed_at\" AS t1_r10 FROM \"reports\" INNER JOIN \"hosts\" ON \"hosts\".\"id\" = \"reports\".\"host_id\" AND \"hosts\".\"type\" IN ('Host::Managed') INNER JOIN \"taxonomies\" ON \"taxonomies\".\"id\" = \"hosts\".\"organization_id\" AND \"taxonomies\".\"type\" IN ('Organization') INNER JOIN \"hosts\" \"hosts_reports\" ON \"hosts_reports\".\"id\" = \"reports\".\"host_id\" AND \"hosts_reports\".\"type\" IN ('Host::Managed') WHERE \"reports\".\"type\" IN ('ConfigReport') AND \"taxonomies\".\"type\" IN ('Organization') AND (\"taxonomies\".\"id\" = 114267492 OR \"taxonomies\".\"title\" = '114267492') AND (1=1) AND ((\"taxonomies\".\"name\" = '114267492')) AND (1=1) ORDER BY \"reports\".\"reported_at\" DESC LIMIT 20 OFFSET 0"

I think AND ((\"taxonomies\".\"name\" = '114267492')) is the culprit here, but not sure yet where that comes from.

@bastilian bastilian force-pushed the bastilian:fix/25097-2 branch 2 times, most recently from 0c90a18 to 86341b3 Dec 6, 2018

@tbrisker

This comment has been minimized.

Copy link
Member

tbrisker commented Dec 6, 2018

[test katello]

@tbrisker
Copy link
Member

tbrisker left a comment

looking good, found one issue though

@@ -5,7 +5,7 @@ class ConfigReportsController < V2::BaseController
include Foreman::Controller::SmartProxyAuth

before_action :find_resource, :only => %w{show destroy}
before_action :setup_search_options, :only => [:index, :last]
before_action :setup_search_options, :only => [:last]

This comment has been minimized.

Copy link
@tbrisker

tbrisker Dec 6, 2018

Member

This has an interesting side affect on api/v2/hosts/:id/config_reports. while the reports are still correctly filtered, the subtotal field shows the wrong number, same as total, instead of total for the selected host

@bastilian bastilian force-pushed the bastilian:fix/25097-2 branch from 86341b3 to a7b1b64 Dec 10, 2018

@bastilian

This comment has been minimized.

Copy link
Member Author

bastilian commented Dec 10, 2018

[test foreman] The FactValue API tests pass locally. Running foreman tests again.

@tbrisker
Copy link
Member

tbrisker left a comment

Thanks @bastilian !

@tbrisker tbrisker merged commit 4cf21db into theforeman:develop Dec 11, 2018

6 of 7 checks passed

upgrade Build finished. No test results found.
Details
Hound No violations found. Woof!
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
foreman Build finished. 36772 tests run, 5 skipped, 0 failed.
Details
katello Build finished. 4116 tests run, 9 skipped, 0 failed.
Details
prprocessor Commit message style is correct
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.