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 #24801 - Always seed default taxonomies #6283

Merged
merged 1 commit into from Dec 18, 2018

Conversation

Projects
None yet
7 participants
@tbrisker
Copy link
Member

tbrisker commented Nov 27, 2018

This is needed so we can enforce enabling taxonomies. If this seed is
run on an existing system, all existing objects will be associated to
the default taxonomy. For systems that already have taxonomies this
seed will remain a noop.

@theforeman-bot

This comment has been minimized.

Copy link
Member

theforeman-bot commented Nov 27, 2018

Issues: #24801

@@ -7,8 +7,6 @@ class Taxonomy < ApplicationRecord

serialize :ignore_types, Array

belongs_to :user

This comment has been minimized.

Copy link
@tbrisker

tbrisker Nov 27, 2018

Author Member

dead code leftovers afaict

@@ -13,9 +13,6 @@
config.consider_all_requests_local = false
config.action_controller.perform_caching = true

# Eager load all classes pre-fork for performance
config.eager_load = true

This comment has been minimized.

Copy link
@tbrisker

tbrisker Nov 27, 2018

Author Member

we already set this on line 10

@tbrisker tbrisker force-pushed the tbrisker:24801 branch 4 times, most recently from 623fa2a to a40025c Nov 27, 2018

@tbrisker

This comment has been minimized.

Copy link
Member Author

tbrisker commented Nov 28, 2018

[test katello]

@tbrisker

This comment has been minimized.

Copy link
Member Author

tbrisker commented Nov 28, 2018

Katello failure is related, @rahulbajaj0509 is looking into updating the katello seeds to match this change.
Would be good to get review for this code anyways ;)

@ohadlevy

This comment has been minimized.

Copy link
Member

ohadlevy commented Nov 28, 2018

I wonder if this is enough for overall migration, or do we need some notifications, ui / tour or something to help customers (similar to the codebase that runs when we initially enabled taxonomies e.g. https://github.com/theforeman/foreman/blob/develop/app/controllers/application_controller.rb#L332)?

@xprazak2

This comment has been minimized.

Copy link
Contributor

xprazak2 commented Dec 3, 2018

Custom partition table templates were not assigned to organization/location when I tested.
Steps to reproduce:

  1. install foreman
  2. clone several partition table templates
  3. apply patch
  4. enable taxonomies
  5. restart apache
  6. templates cloned in step 2 will not have taxonomies assigned
@tbrisker

This comment has been minimized.

Copy link
Member Author

tbrisker commented Dec 9, 2018

Updated to assign all templates to the new taxonomies.

@mmoll

This comment has been minimized.

Copy link
Member

mmoll commented Dec 9, 2018

katello test failure related? (update: sorry, already known)

@tbrisker

This comment has been minimized.

Copy link
Member Author

tbrisker commented Dec 9, 2018

@mmoll it is, the related katello pr is needed for it to pass

@tbrisker

This comment has been minimized.

Copy link
Member Author

tbrisker commented Dec 12, 2018

I wonder if this is enough for overall migration, or do we need some notifications, ui / tour or something to help customers (similar to the codebase that runs when we initially enabled taxonomies e.g. https://github.com/theforeman/foreman/blob/develop/app/controllers/application_controller.rb#L332)?

@ohadlevy - I think this is out of scope for this PR, we don't currently have a framework in place for this nor would I want to create another notification blueprint for just one off - the users upgrading from previous versions will have already gotten a notification about this in 1.20 if they disabled the setting.
TBH, i think the existing path is more painful - it required extra steps before you could use a freshly installed foreman, rather than just creating the default taxonomies and letting the users later add/change their setup if needed.

@xprazak2

This comment has been minimized.

Copy link
Contributor

xprazak2 commented Dec 12, 2018

Looking good, everything got assigned to the default taxonomies when I tested.

@ares
Copy link
Member

ares left a comment

Do we have some strategy for resources introduced in plugins? Some howto might be handy.

end

# Only default templates are assigned during taxonomy creation
Template.where(default: false).each do |template|

This comment has been minimized.

Copy link
@ares

ares Dec 17, 2018

Member

should we associate all templates? if we didn't have any taxonomy before, it behaved like it was available globally right? which is the new default taxonomy purpose

This comment has been minimized.

Copy link
@tbrisker

tbrisker Dec 17, 2018

Author Member

The default ones are already assigned when the taxonomy is created by https://github.com/theforeman/foreman/blob/develop/app/models/taxonomy.rb#L12

@tbrisker

This comment has been minimized.

Copy link
Member Author

tbrisker commented Dec 17, 2018

@ares - regarding resources introduced in plugins, they will also be assigned by the same seed if they properly defined the relationships between the models since we are using taxonomy.reflect_on_all_associations.reject {|assoc| skip_associations.include?(assoc.name)} to iterate over all associations except those we explicitly skip. If any resources are missing because they weren't properly defined, it's very simple to assign them manually if needed.

Fixes #24801 - Always seed default taxonomies
This is needed so we can enforce enabling taxonomies. If this seed is
run on an existing system, all existing objects will be associated to
the default taxonomy. For systems that already have taxonomies this
seed will remain a noop.

@tbrisker tbrisker force-pushed the tbrisker:24801 branch from 3b1af0f to 6b918f8 Dec 18, 2018

@ares

ares approved these changes Dec 18, 2018

Copy link
Member

ares left a comment

Thanks @tbrisker, merging!

@ares ares added Ready to merge and removed Needs testing labels Dec 18, 2018

@ares ares merged commit ddfc231 into theforeman:develop Dec 18, 2018

6 of 7 checks passed

katello Build finished. 4118 tests run, 9 skipped, 1 failed.
Details
Hound No violations found. Woof!
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
foreman Build finished. 36061 tests run, 15 skipped, 0 failed.
Details
prprocessor Commit message style is correct
Details
upgrade Build finished. No test results found.
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.