Fixes #18639 - lock all templates we seed #4320

ares · 2017-02-23T12:13:54Z

The PR looks big but what it does is very simple

extracts names into constants and into separate seed files so they can be reused safely in migration without triggering the seed
adds a migration that sets locked flag for seeded templates to true
makes sure all newly seeded templates are locked

For more information and motivations, please see the discussion on foreman-dev list

mention-bot · 2017-02-23T12:13:55Z

@ares, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dLobatog, @domcleal and @stbenjam to be potential reviewers.

houndci-bot · 2017-02-23T12:14:07Z

db/seeds.d/07-data.rb

+
+# Template kinds
+kinds = {}
+TemplateKind.default_template_labels.keys.collect(&:to_sym).each do |type|


Prefer map over collect.

houndci-bot · 2017-02-23T12:14:07Z

db/seeds.d/07-data.rb

+
+# Template kinds
+kinds = {}
+TemplateKind.default_template_labels.keys.collect(&:to_sym).each do |type|


Prefer map over collect.

timogoebel · 2017-02-23T19:11:42Z

@ares: How about taking a more radical approach and rename all seeded templates that have previously been modified so we can make sure all original templates are present and locked. I think, we should go all the way with this.

timogoebel · 2017-02-23T19:13:58Z

db/seeds.d/07-provisioning_templates.rb

@@ -105,7 +17,8 @@

      t = ProvisioningTemplate.create({
        :snippet  => false,
-        :template => contents
+        :template => contents,
+        :locked => true
      }.merge(input))

      if t.default?


I know, this is unrelated to this PR, but isn't t.default? always true here?

yes, it's always set like that few lines above, I'll remove the if condition while I'm at it

ares · 2017-02-24T08:48:02Z

How about taking a more radical approach and rename all seeded templates that have previously been modified so we can make sure all original templates are present and locked. I think, we should go all the way with this.

@timogoebel I'm afraid this won't work reliably for snippets, their names are hardcoded in templates. I'd like to avoid updating their names in existing templates. Since we override default templates with every seed (not sure for how many versions) I hope there won't be custom changes in seeded templates. If renaming is preferred, maybe we could do it for all templates except snippets?

dLobatog · 2017-03-14T12:14:32Z

Tested and 👍 to the change once Katello tests are green. I was a bit confused by one thing:

If I modified the "PXElinux default template" because I wanted to add custom stuff, then update Foreman to a version with this, my change remains in the locked template, but if I want to keep it i should clone it or rename it, correct?

If that's the case, still 👍 but let's warn about it in the release and upgrade notes

timogoebel · 2017-03-14T12:21:37Z

If I modified the "PXElinux default template" because I wanted to add custom stuff, then update Foreman to a version with this, my change remains in the locked template, but if I want to keep it i should clone it or rename it, correct?

Actually, I think this is still bad design. I think, we should only lock seeded templates and not continue with this mess.
Let's ship locked default templates managed by us and users can "fork" them if they choose to do so.

ares · 2017-03-14T12:23:54Z

I think that's what the PR does. @dLobatog if you want to keep such changes to survive the update, you need to rename it anyway because we currently override any custom changes on rake db:seed.

dLobatog · 2017-03-14T12:47:44Z

@timogoebel That's what this PR is for 😄 the locking functionality was there because Katello did this for a long time already.

houndci-bot · 2017-03-17T15:50:28Z

app/models/template.rb

@@ -57,6 +58,13 @@ def filename
    name.downcase.delete('-').gsub(/\s+/, '_') + '.erb'
  end

+  def ignore_locking(&block)
+    self.modify_locked = true
+    block.call(self)


Use yield instead of block.call.

houndci-bot · 2017-03-17T15:57:12Z

app/models/template.rb

        errors.add(:base, _("You are not authorized to lock templates."))
      end
    end

-    if actual_changes.include? 'default'
-      unless User.current.can?(:create_organizations) || User.current.can?(:create_locations)
+    if actual_changes.include? 'default' && !self.modify_default


Use parentheses in the method call to avoid confusion about precedence.

houndci-bot · 2017-03-17T15:57:12Z

app/models/template.rb

@@ -71,19 +86,19 @@ def template_changes
    actual_changes = changes

    # Locked & Default are Special
-    if actual_changes.include? 'locked'
-      unless User.current.can?("lock_#{self.class.to_s.underscore.pluralize}", self)
+    if actual_changes.include? 'locked' && !self.modify_locked


Use parentheses in the method call to avoid confusion about precedence.

ares · 2017-03-23T09:27:28Z

Rebased to avoid compute resource test failures, should be green now.

timogoebel · 2017-03-23T10:07:20Z

@dLobatog / @tbrisker : Does anyone of you have time to check this one out? We should get this in. :-)

ares · 2017-03-23T11:38:08Z

Foreman issues seems unrelated

HostsControllerTest.test_0070_select multiple action with valid host_ids param should return a selection page (pg/ruby 2.4.0)
ComputeResourceJSIntegrationTest.test_0003_compute resource password isn't deleted when testing connection (pg/ruby 2.0.0)

ares · 2017-03-23T14:58:18Z

[test katello]

ares · 2017-03-24T17:43:21Z

Rebased again

tbrisker · 2017-03-26T06:52:26Z

code-wise looks good, waiting for feedback from @dLobatog since he reviewed this more deeply.

dLobatog

@ares 👍 from me again, I just have 2 questions you can reply to inline. Thanks for this!

dLobatog · 2017-03-28T07:50:07Z

app/models/template.rb

@@ -1,5 +1,6 @@
 class Template < ActiveRecord::Base
  include Exportable
+  attr_accessor :modify_locked, :modify_default


How about making this an attr_reader in order to prevent modifications outside of this class (using the ignore_locking/ignore_default helpers)?

yeah I could do that but I though people might find it useful e.g. in tests when they just disable locking in setup and re-enable in teardown even though the ignore_locking is preferable elsewhere

dLobatog · 2017-03-28T07:52:30Z

db/migrate/20170223114114_lock_seeded_templates.rb

+class LockSeededTemplates < ActiveRecord::Migration
+  def up
+    names = (SEEDED_TEMPLATES + SEEDED_PARTITION_TABLES).map { |attrs| attrs[:name] }
+    Template.where(:name => names).update_all(:locked => true)


Why is this needed if :locked => true is set in the seed https://github.com/theforeman/foreman/pull/4320/files#diff-151633105c29141fe3aac7603de185a5R25 ? Is this only relevant for partition tables?

On existing db we need to lock all templates, they were seeded unlocked in previous versions. So we find all templates by name that would be locked in new installation and lock them.

dLobatog · 2017-03-28T08:01:03Z

Thanks @ares & @tbrisker @timogoebel !

theforeman-bot added Needs testing Not yet reviewed labels Feb 23, 2017

houndci-bot reviewed Feb 23, 2017

View reviewed changes

ares force-pushed the fix/18639 branch from cad715b to dca5e24 Compare February 23, 2017 13:07

ares mentioned this pull request Feb 23, 2017

Fixes #18640 - realign templates structure #4322

Merged

timogoebel reviewed Feb 23, 2017

View reviewed changes

ares mentioned this pull request Feb 23, 2017

Fixes #17228 - Select templates as global defaults #4283

Merged

ares force-pushed the fix/18639 branch from dca5e24 to dc64e74 Compare February 24, 2017 12:53

ares force-pushed the fix/18639 branch from dc64e74 to 5367afc Compare March 3, 2017 13:21

dLobatog added Waiting on contributor and removed Needs testing Not yet reviewed labels Mar 14, 2017

ares force-pushed the fix/18639 branch from 5367afc to fb7c229 Compare March 17, 2017 15:50

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Mar 17, 2017

houndci-bot reviewed Mar 17, 2017

View reviewed changes

ares force-pushed the fix/18639 branch 2 times, most recently from 3a3e947 to 56728d8 Compare March 17, 2017 15:56

houndci-bot reviewed Mar 17, 2017

View reviewed changes

ares force-pushed the fix/18639 branch 2 times, most recently from 7cde2d2 to b1cccdb Compare March 23, 2017 09:27

Fixes #18639 - lock all templates we seed

0bf4686

ares force-pushed the fix/18639 branch from b1cccdb to 0bf4686 Compare March 24, 2017 17:41

tbrisker added the 1.15.0 label Mar 26, 2017

dLobatog approved these changes Mar 28, 2017

View reviewed changes

dLobatog merged commit 4a98e18 into theforeman:develop Mar 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #18639 - lock all templates we seed #4320

Fixes #18639 - lock all templates we seed #4320

ares commented Feb 23, 2017

mention-bot commented Feb 23, 2017

houndci-bot Feb 23, 2017

houndci-bot Feb 23, 2017

timogoebel commented Feb 23, 2017

timogoebel Feb 23, 2017

ares Feb 24, 2017

ares commented Feb 24, 2017

dLobatog commented Mar 14, 2017

timogoebel commented Mar 14, 2017

ares commented Mar 14, 2017

dLobatog commented Mar 14, 2017

houndci-bot Mar 17, 2017

houndci-bot Mar 17, 2017

houndci-bot Mar 17, 2017

ares commented Mar 23, 2017

timogoebel commented Mar 23, 2017

ares commented Mar 23, 2017

ares commented Mar 23, 2017

ares commented Mar 24, 2017

tbrisker commented Mar 26, 2017

dLobatog left a comment

dLobatog Mar 28, 2017

ares Mar 28, 2017

dLobatog Mar 28, 2017

ares Mar 28, 2017

dLobatog commented Mar 28, 2017

Fixes #18639 - lock all templates we seed #4320

Fixes #18639 - lock all templates we seed #4320

Conversation

ares commented Feb 23, 2017

mention-bot commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timogoebel commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ares commented Feb 24, 2017

dLobatog commented Mar 14, 2017

timogoebel commented Mar 14, 2017

ares commented Mar 14, 2017

dLobatog commented Mar 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ares commented Mar 23, 2017

timogoebel commented Mar 23, 2017

ares commented Mar 23, 2017

ares commented Mar 23, 2017

ares commented Mar 24, 2017

tbrisker commented Mar 26, 2017

dLobatog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dLobatog commented Mar 28, 2017