Tool to replicate server settings to other servers #15990

jameswnl · 2017-09-18T22:15:37Z

This is the second part of the pivotal story

jameswnl · 2017-09-19T13:12:40Z

@bronaghs @gtanzillo can you help review this?

thanks

bronaghs · 2017-09-19T14:37:52Z

tools/replicate_server_settings.rb

+  target_settings = target.get_config("vmdb")
+  target_settings.config.store_path(*path, values)
+
+  valid, errors = VMDB::Config::Validator.new(settings).validate


@jameswnl - should this read:
VMDB::Config::Validator.new(target_settings).validate ?

Thanks for spotting it! Done now.

bronaghs · 2017-09-19T14:42:22Z

Just one question inline @jameswnl
Can you add a PR description.
We want to add videos where possible to help other teams understand new features, maybe you could add a video for this.

jameswnl · 2017-09-19T15:23:07Z

@miq-bot remove_label wip

carbonin · 2017-09-19T21:20:06Z

tools/replicate_server_settings.rb

+server = opts[:serverid] ? MiqServer.find(opts[:serverid]) : MiqServer.my_server
+settings = server.get_config("vmdb")
+
+config = settings.config


I think the better way to do this would be server.settings_for_resource

carbonin · 2017-09-19T21:27:38Z

tools/replicate_server_settings.rb

+target_servers.each do |target|
+  puts " - replicating to server id=#{target.id}..."
+  target_settings = target.get_config("vmdb")
+  target_settings.config.store_path(*path, values)


Similar thing here, I think you can make use of target.add_settings_for_resource directly rather than going through the set_config method.

Thanks! Done

carbonin · 2017-09-19T21:30:17Z

tools/replicate_server_settings.rb

+  target_settings = target.get_config("vmdb")
+  target_settings.config.store_path(*path, values)
+
+  valid, errors = VMDB::Config::Validator.new(target_settings).validate


You shouldn't need to do this, add_settings_for_resource calls Vmdb::Settings.save! which does validation for you, plus I don't see invalid config being much of an issue as the settings you're saving have already been saved for a different server 😉

great! Done

carbonin · 2017-09-19T21:31:47Z

tools/replicate_server_settings.rb

+
+opts = Trollop.options(ARGV) do
+  banner "USAGE:   #{__FILE__} -s <server id> -p <settings path separated by a /> \n" \
+         "Example: #{__FILE__} -d -s 1 -p ems/ems_amazon/additiona_instance_types"


typo, missing the l in "additional"

carbonin · 2017-09-19T21:32:12Z

tools/replicate_server_settings.rb

+require 'trollop'
+
+opts = Trollop.options(ARGV) do
+  banner "USAGE:   #{__FILE__} -s <server id> -p <settings path separated by a /> \n" \


I feel like something like path/to/the/settings might be more clear? Especially because /> looks like some kind of closing tag, so it might get ignored.

https://www.pivotaltracker.com/n/projects/1644071/stories/149319063

carbonin

Could we put these methods into a class so that we can write specs for them?

We do this for other high impact tools such as fix_auth and column_ordering.

carbonin · 2017-09-20T17:43:57Z

tools/replicate_server_settings.rb

+
+def construct_setting_tree(path, values)
+  # construct the partial tree containing the target values
+  path.reverse.inject(values) { |m, e| {e => m} }


What are the meanings of m and e here? Merged and element? I feel like names would help this to be more readable.

carbonin · 2017-09-20T17:44:57Z

tools/replicate_server_settings.rb

+  target_servers.each do |target|
+    puts " - replicating to server id=#{target.id}..."
+    target.add_settings_for_resource(target_settings)
+    target.save!


I don't think you need to save target here. We are not actually changing anything on that object.

carbonin · 2017-09-20T17:49:39Z

tools/replicate_server_settings.rb

+puts opts.inspect
+Trollop.die :path, "is required" unless opts[:path_given]
+
+def replicate(opts)


I think this would be better not using an options hash.

Then you could collapse the first line into a default value:

def replicate(path_string, dry_run, server = MiqServer.my_server) ... end

Clearly the caller of replicate would have to change also (to look up the server if given an id, but I don't think that's so bad. Or you could put the "find the source server" logic outside of the replicate method entirely.

miq-bot · 2017-09-20T19:42:25Z

Checked commits jameswnl/manageiq@6e7206b~...7af94f6 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 👍

carbonin · 2017-09-20T19:45:23Z

spec/tools/server_settings_replicator/server_settings_replicator_spec.rb

+  describe "#replicate" do
+    it "targets only other servers" do
+      miq_server.add_settings_for_resource(settings)
+      expect(described_class).to receive(:copy_to).with([miq_server_remote], settings)


Is there any reason that we can't actually check that the setting was saved for the other server here?

Why stub out the call to .copy_to?

wanna check the arguments being passed to it.

I was thinking do we need to check the outcome of add_settings_for_resource (in copy_to)? It should be covered by the configuration management aspect.

Right, but you're not actually testing that the settings were saved. I'm okay with this either way, I just lean towards not stubbing things if I don't have to.

I guess the logic here is, copy_to is simple enough and add_settings_for_resource is already tested...

I'm okay, with this as is.

right, that's the thinking
👍

I actually wrote my feedback for your followup PR before seeing this. I agree, I think the mock here is hiding a larger issue with your class interface and test boundaries: #16023 (review)

jameswnl · 2017-09-20T20:27:32Z

@carbonin thanks for the review!

carbonin · 2017-09-20T20:28:58Z

Just waiting on coveralls ...

carbonin · 2017-09-20T21:23:16Z

Coveralls is taking too long.

chrisarcand · 2017-09-21T19:42:32Z

tools/server_settings_replicator/server_settings_replicator.rb

+    settings = construct_setting_tree(path, server.settings_for_resource.fetch_path(path).to_h)
+
+    puts "Replicating from server id=#{server.id}, path=#{path_string} to #{target_servers.count} servers"
+    puts "Settings: #{settings}"


NoooOooooOoooooooo

good catch! This was started as a simple tool script that no spec was intended and it outgrew...
I have another PR to silent them coming.

The PR is #16023

bronaghs · 2018-03-15T19:46:47Z

@jameswnl - Can you see any reason why this cannot be backported to FINE?
cc @simaishi

jameswnl · 2018-03-15T19:48:28Z

@bronaghs this should be portable.

bronaghs · 2018-03-15T19:59:27Z

@simaishi - If we are backporting the first part of the pivotal story (see description above and PR ManageIQ/manageiq-providers-amazon#289) I think we should also backport this PR too

Fine BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1557025

Tool to replicate server settings to other servers (cherry picked from commit a215bb7) https://bugzilla.redhat.com/show_bug.cgi?id=1557025

simaishi · 2018-04-09T18:35:30Z

Fine backport details:

$ git log -1
commit 1efe7a4aff9ce4d2027c9de2fd0f5715b9a5eb46
Author: Nick Carboni <[email protected]>
Date:   Wed Sep 20 17:23:27 2017 -0400

    Merge pull request #15990 from jameswnl/rep-server-settings
    
    Tool to replicate server settings to other servers
    (cherry picked from commit a215bb733535f250c58c18c555793b9b09cc805e)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1557025

Tool to replicate server settings to other servers (cherry picked from commit a215bb7) https://bugzilla.redhat.com/show_bug.cgi?id=1557025

jameswnl force-pushed the rep-server-settings branch from f794daf to e48ebdf Compare September 18, 2017 22:16

miq-bot added the wip label Sep 18, 2017

jameswnl force-pushed the rep-server-settings branch 2 times, most recently from 0fdaf0e to 83b4720 Compare September 19, 2017 01:02

chessbyte added the tools label Sep 19, 2017

chessbyte changed the title ~~[WIP]Tool to replicate server settings to other servers~~ [WIP] Tool to replicate server settings to other servers Sep 19, 2017

bronaghs reviewed Sep 19, 2017

View reviewed changes

jameswnl force-pushed the rep-server-settings branch 2 times, most recently from e507072 to 08663ca Compare September 19, 2017 15:22

jameswnl changed the title ~~[WIP] Tool to replicate server settings to other servers~~ Tool to replicate server settings to other servers Sep 19, 2017

miq-bot removed the wip label Sep 19, 2017

chessbyte assigned gtanzillo Sep 19, 2017

chessbyte added the core label Sep 19, 2017

carbonin suggested changes Sep 19, 2017

View reviewed changes

jameswnl force-pushed the rep-server-settings branch from 08663ca to 5caa5dc Compare September 20, 2017 16:04

Tool to replicate server settings to other servers

6e7206b

https://www.pivotaltracker.com/n/projects/1644071/stories/149319063

jameswnl force-pushed the rep-server-settings branch from 5caa5dc to 6e7206b Compare September 20, 2017 16:05

carbonin reviewed Sep 20, 2017

View reviewed changes

Refac and add spec for replicate server settings tool

7af94f6

jameswnl force-pushed the rep-server-settings branch from 7971b89 to 7af94f6 Compare September 20, 2017 19:38

carbonin reviewed Sep 20, 2017

View reviewed changes

carbonin approved these changes Sep 20, 2017

View reviewed changes

carbonin assigned carbonin and unassigned gtanzillo Sep 20, 2017

carbonin added this to the Sprint 70 Ending Oct 2, 2017 milestone Sep 20, 2017

carbonin merged commit a215bb7 into ManageIQ:master Sep 20, 2017

jameswnl deleted the rep-server-settings branch September 21, 2017 00:51

chrisarcand reviewed Sep 21, 2017

View reviewed changes

cybette mentioned this pull request Sep 26, 2017

2017-09-25 LWIMIQ ManageIQ/manageiq.org#589

Merged

kbrock mentioned this pull request Feb 26, 2018

Silence replicator spec puts #17053

Merged

simaishi added the fine/yes label Mar 15, 2018

simaishi pushed a commit that referenced this pull request Apr 9, 2018

Merge pull request #15990 from jameswnl/rep-server-settings

1efe7a4

Tool to replicate server settings to other servers (cherry picked from commit a215bb7) https://bugzilla.redhat.com/show_bug.cgi?id=1557025

simaishi added fine/backported and removed fine/yes labels Apr 9, 2018

Tool to replicate server settings to other servers #15990

Tool to replicate server settings to other servers #15990

Conversation

jameswnl commented Sep 18, 2017 • edited Loading

jameswnl commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bronaghs commented Sep 19, 2017

jameswnl commented Sep 19, 2017

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

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

carbonin 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Sep 20, 2017

Choose a reason for hiding this comment

jameswnl Sep 20, 2017 • edited Loading

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

jameswnl commented Sep 20, 2017

carbonin commented Sep 20, 2017

carbonin commented Sep 20, 2017

chrisarcand Sep 21, 2017 • edited Loading

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

bronaghs commented Mar 15, 2018

jameswnl commented Mar 15, 2018

bronaghs commented Mar 15, 2018

simaishi commented Apr 9, 2018

jameswnl commented Sep 18, 2017 •

edited

Loading

jameswnl Sep 20, 2017 •

edited

Loading

chrisarcand Sep 21, 2017 •

edited

Loading