From f194d3587bc46f3ba0aa20dd7cec7e223b2f0304 Mon Sep 17 00:00:00 2001 From: David Roetzel Date: Thu, 11 May 2023 10:17:37 +0200 Subject: [PATCH 1/3] Add `diffy` to create nice html diffs of values #151 --- Gemfile | 1 + Gemfile.lock | 2 ++ 2 files changed, 3 insertions(+) diff --git a/Gemfile b/Gemfile index 8078ee0e..de94d704 100644 --- a/Gemfile +++ b/Gemfile @@ -34,6 +34,7 @@ gem 'net-ldap', require: "net/ldap" gem 'breadcrumbs_on_rails' gem 'cancancan' gem 'ruby-saml' +gem 'diffy' # To use retry middleware with Faraday v2.0+ gem 'faraday-retry' diff --git a/Gemfile.lock b/Gemfile.lock index 8057c527..6bc5f5bc 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -125,6 +125,7 @@ GEM railties (>= 6.0.0) date (3.3.3) deep_merge (1.2.2) + diffy (3.4.2) docile (1.4.0) erubi (1.12.0) execjs (2.8.1) @@ -423,6 +424,7 @@ DEPENDENCIES cancancan capybara (>= 2.15) dartsass-rails + diffy factory_bot_rails faker faraday-retry From f939984ae54d5beb0a4a55e997e8abf6ab52ea61 Mon Sep 17 00:00:00 2001 From: David Roetzel Date: Wed, 31 May 2023 15:10:32 +0200 Subject: [PATCH 2/3] Display value differences to actual env #151 If a key is displayed from a different environment than the node is actually in _and_ a file with the same path exists in the original environment _and_ the value in that file differs from the one displayed, show the differences. Uses the `diffy` gem to highlight the differences. --- app/assets/stylesheets/application.scss | 15 +++++ app/models/data_file.rb | 29 ++++++++++ app/views/keys/_diff.html.erb | 15 +++++ app/views/keys/_value.html.erb | 22 +++++++ app/views/keys/show.html.erb | 2 +- .../development/data/role/hdm_test.yaml | 2 + .../globs/data/role/hdm_test.yaml | 2 + test/integration/key_search_test.rb | 4 +- test/models/data_file_test.rb | 58 +++++++++++++++++++ 9 files changed, 146 insertions(+), 3 deletions(-) create mode 100644 app/views/keys/_diff.html.erb create mode 100644 app/views/keys/_value.html.erb create mode 100644 test/models/data_file_test.rb diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss index b666bbff..036a839b 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -41,3 +41,18 @@ ol.breadcrumb li:first-child { width: 1em; height: 1em; } + +// Diffy Styles +.diff{overflow:auto;} +.diff ul{background:#fff;overflow:auto;font-size:13px;list-style:none;margin:0;padding:0;display:table;width:100%;} +.diff del, .diff ins{display:block;text-decoration:none;} +.diff li{padding:0; display:table-row;margin: 0;height:1em;} +.diff li.ins{background:#dfd; color:#080} +.diff li.del{background:#fee; color:#b00} +.diff li:hover{background:#ffc} +/* try 'whitespace:pre;' if you don't want lines to wrap */ +.diff del, .diff ins, .diff span{white-space:pre-wrap;font-family:courier;} +.diff del strong{font-weight:normal;background:#fcc;} +.diff ins strong{font-weight:normal;background:#9f9;} +.diff li.diff-comment { display: none; } +.diff li.diff-block-info { background: none repeat scroll 0 0 gray; } diff --git a/app/models/data_file.rb b/app/models/data_file.rb index 0c5fc756..be19bd9f 100644 --- a/app/models/data_file.rb +++ b/app/models/data_file.rb @@ -66,4 +66,33 @@ def replaced_from_git? def to_param path end + + def id + [node&.name, path].join("-").parameterize + end + + def has_differing_value_in_original_environment?(key) + return false unless environment && node.environment + return false if environment == node.environment + candidate_files = node.environment.hierarchies.map do |h| + self.class.new(hierarchy: h, path: path, node: node) + end + other_file = candidate_files.find { |f| f.exist? } + return false unless other_file + other_key = Key.new(environment: node.environment, name: key.name) + return false if value_for(key: key).value == other_file.value_for(key: other_key).value + true + end + + def value_from_original_environment(key:) + candidate_files = (node&.environment&.hierarchies || []).map do |h| + self.class.new(hierarchy: h, path: path, node: node) + end + other_file = candidate_files.find { |f| f.exist? } + other_key = Key.new(environment: node.environment, name: key.name) + other_file.value_for(key: other_key) + end + + private + end diff --git a/app/views/keys/_diff.html.erb b/app/views/keys/_diff.html.erb new file mode 100644 index 00000000..5f7f40f1 --- /dev/null +++ b/app/views/keys/_diff.html.erb @@ -0,0 +1,15 @@ +<% split_diff = Diffy::SplitDiff.new(file.value_from_original_environment(key: @key).value, value.value, format: :html) %> + + + + + + + + + + + + + +
<%= @node.environment.name %><%= @environment.name %>
<%= raw(split_diff.left) %><%= raw(split_diff.right) %>
diff --git a/app/views/keys/_value.html.erb b/app/views/keys/_value.html.erb new file mode 100644 index 00000000..db409ade --- /dev/null +++ b/app/views/keys/_value.html.erb @@ -0,0 +1,22 @@ +<% if file.has_differing_value_in_original_environment?(@key) %> + + +
+
" class="tab-pane fade show active" role="tabpanel" aria-labelledby="<%= "tab-value-#{dom_id(file)}" %>"> + <%= render "form", file: file, value: value %> +
+ +
" class="tab-pane fade" role="tabpanel" aria-labelledby="<%= "tab-diff-#{dom_id(file)}" %>"> + <%= render "diff", file: file, value: value %> +
+
+<% else %> + <%= render "form", file: file, value: value %> +<% end %> diff --git a/app/views/keys/show.html.erb b/app/views/keys/show.html.erb index 2c9afe3a..fe5a29c0 100644 --- a/app/views/keys/show.html.erb +++ b/app/views/keys/show.html.erb @@ -55,7 +55,7 @@
- <%= render "form", file: file, value: value %> + <%= render "value", file: file, value: value %>
diff --git a/test/fixtures/files/puppet/environments/development/data/role/hdm_test.yaml b/test/fixtures/files/puppet/environments/development/data/role/hdm_test.yaml index 14e8fa1e..59a995cf 100644 --- a/test/fixtures/files/puppet/environments/development/data/role/hdm_test.yaml +++ b/test/fixtures/files/puppet/environments/development/data/role/hdm_test.yaml @@ -1,3 +1,5 @@ --- foobar::firstrun::linux_classes: hostname: hostname-role +hdm::integer: 1 +hdm::float: 2.3 diff --git a/test/fixtures/files/puppet/environments/globs/data/role/hdm_test.yaml b/test/fixtures/files/puppet/environments/globs/data/role/hdm_test.yaml index 14e8fa1e..a1b8f1dc 100644 --- a/test/fixtures/files/puppet/environments/globs/data/role/hdm_test.yaml +++ b/test/fixtures/files/puppet/environments/globs/data/role/hdm_test.yaml @@ -1,3 +1,5 @@ --- foobar::firstrun::linux_classes: hostname: hostname-role +hdm::integer: 1 +hdm::float: 4.5 diff --git a/test/integration/key_search_test.rb b/test/integration/key_search_test.rb index 7770eec5..b4d2c463 100644 --- a/test/integration/key_search_test.rb +++ b/test/integration/key_search_test.rb @@ -6,11 +6,11 @@ class KeySearchTest < ActionDispatch::IntegrationTest sign_in_as(@user) end - test "user withouth groups can find key" do + test "user without groups can find key" do get environment_key_files_path(environment_id: "development", key_id: "hdm::integer") assert_response :success - assert_select "p.lead", /Found\s+key\s+hdm::integer\s+in\s+1\s+file./ + assert_select "p.lead", /Found\s+key\s+hdm::integer\s+in\s+2\s+files./ end test "user with group that forbids access cannot find key" do diff --git a/test/models/data_file_test.rb b/test/models/data_file_test.rb new file mode 100644 index 00000000..c61352cd --- /dev/null +++ b/test/models/data_file_test.rb @@ -0,0 +1,58 @@ +require 'test_helper' + +class DataFileTest < ActiveSupport::TestCase + setup do + @original_environment = Environment.new(name: "development") + @node = Node.new(environment: @original_environment, hostname: "test.host") + @globs_environment = Environment.new(name: "globs") + @path = "role/hdm_test.yaml" + end + + test "#has_differing_value_in_original_environment? returns false if environments are the same" do + hierarchy = Hierarchy.find(@original_environment, "Eyaml hierarchy") + data_file = DataFile.new(hierarchy: hierarchy, node: @node, path: @path) + key = Key.new(environment: @original_environment, name: "hdm::integer") + + assert_not data_file.has_differing_value_in_original_environment?(key) + end + + test "#has_differing_value_in_original_environment? returns false if original environment has no file with the same path" do + hierarchy = Hierarchy.find(@globs_environment, "Common") + data_file = DataFile.new(hierarchy: hierarchy, node: @node, path: "common/foobar.yaml") + key = Key.new(environment: @globs_environment, name: "hdm::integer") + + assert_not data_file.has_differing_value_in_original_environment?(key) + end + + test "#has_differing_value_in_original_environment? returns false if original environment's value is the same" do + hierarchy = Hierarchy.find(@globs_environment, "Eyaml hierarchy") + data_file = DataFile.new(hierarchy: hierarchy, node: @node, path: @path) + key = Key.new(environment: @globs_environment, name: "hdm::integer") + + assert_not data_file.has_differing_value_in_original_environment?(key) + end + + test "#has_differing_value_in_original_environment? returns true if original environment's value is different" do + hierarchy = Hierarchy.find(@globs_environment, "Eyaml hierarchy") + data_file = DataFile.new(hierarchy: hierarchy, node: @node, path: @path) + key = Key.new(environment: @globs_environment, name: "hdm::float") + + assert data_file.has_differing_value_in_original_environment?(key) + end + + test "#value_for returns matching value from given environment" do + hierarchy = Hierarchy.find(@globs_environment, "Eyaml hierarchy") + data_file = DataFile.new(hierarchy: hierarchy, node: @node, path: @path) + key = Key.new(environment: @globs_environment, name: "hdm::float") + + assert_equal "4.5", data_file.value_for(key: key).value + end + + test "#value_from_original_environment returns matching value from the node's original environment" do + hierarchy = Hierarchy.find(@globs_environment, "Eyaml hierarchy") + data_file = DataFile.new(hierarchy: hierarchy, node: @node, path: @path) + key = Key.new(environment: @globs_environment, name: "hdm::float") + + assert_equal "2.3", data_file.value_from_original_environment(key: key).value + end +end From 3e1a5a317e6b101a6434db76a1099ca98d091125 Mon Sep 17 00:00:00 2001 From: David Roetzel Date: Wed, 31 May 2023 15:25:37 +0200 Subject: [PATCH 3/3] Fix rubocop issues. --- app/models/data_file.rb | 16 ++++++++-------- test/models/data_file_test.rb | 16 ++++++++-------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/app/models/data_file.rb b/app/models/data_file.rb index be19bd9f..ee27211b 100644 --- a/app/models/data_file.rb +++ b/app/models/data_file.rb @@ -74,25 +74,25 @@ def id def has_differing_value_in_original_environment?(key) return false unless environment && node.environment return false if environment == node.environment + candidate_files = node.environment.hierarchies.map do |h| - self.class.new(hierarchy: h, path: path, node: node) + self.class.new(hierarchy: h, path:, node:) end - other_file = candidate_files.find { |f| f.exist? } + other_file = candidate_files.find(&:exist?) return false unless other_file + other_key = Key.new(environment: node.environment, name: key.name) - return false if value_for(key: key).value == other_file.value_for(key: other_key).value + return false if value_for(key:).value == other_file.value_for(key: other_key).value + true end def value_from_original_environment(key:) candidate_files = (node&.environment&.hierarchies || []).map do |h| - self.class.new(hierarchy: h, path: path, node: node) + self.class.new(hierarchy: h, path:, node:) end - other_file = candidate_files.find { |f| f.exist? } + other_file = candidate_files.find(&:exist?) other_key = Key.new(environment: node.environment, name: key.name) other_file.value_for(key: other_key) end - - private - end diff --git a/test/models/data_file_test.rb b/test/models/data_file_test.rb index c61352cd..63fc103b 100644 --- a/test/models/data_file_test.rb +++ b/test/models/data_file_test.rb @@ -10,7 +10,7 @@ class DataFileTest < ActiveSupport::TestCase test "#has_differing_value_in_original_environment? returns false if environments are the same" do hierarchy = Hierarchy.find(@original_environment, "Eyaml hierarchy") - data_file = DataFile.new(hierarchy: hierarchy, node: @node, path: @path) + data_file = DataFile.new(hierarchy:, node: @node, path: @path) key = Key.new(environment: @original_environment, name: "hdm::integer") assert_not data_file.has_differing_value_in_original_environment?(key) @@ -18,7 +18,7 @@ class DataFileTest < ActiveSupport::TestCase test "#has_differing_value_in_original_environment? returns false if original environment has no file with the same path" do hierarchy = Hierarchy.find(@globs_environment, "Common") - data_file = DataFile.new(hierarchy: hierarchy, node: @node, path: "common/foobar.yaml") + data_file = DataFile.new(hierarchy:, node: @node, path: "common/foobar.yaml") key = Key.new(environment: @globs_environment, name: "hdm::integer") assert_not data_file.has_differing_value_in_original_environment?(key) @@ -26,7 +26,7 @@ class DataFileTest < ActiveSupport::TestCase test "#has_differing_value_in_original_environment? returns false if original environment's value is the same" do hierarchy = Hierarchy.find(@globs_environment, "Eyaml hierarchy") - data_file = DataFile.new(hierarchy: hierarchy, node: @node, path: @path) + data_file = DataFile.new(hierarchy:, node: @node, path: @path) key = Key.new(environment: @globs_environment, name: "hdm::integer") assert_not data_file.has_differing_value_in_original_environment?(key) @@ -34,7 +34,7 @@ class DataFileTest < ActiveSupport::TestCase test "#has_differing_value_in_original_environment? returns true if original environment's value is different" do hierarchy = Hierarchy.find(@globs_environment, "Eyaml hierarchy") - data_file = DataFile.new(hierarchy: hierarchy, node: @node, path: @path) + data_file = DataFile.new(hierarchy:, node: @node, path: @path) key = Key.new(environment: @globs_environment, name: "hdm::float") assert data_file.has_differing_value_in_original_environment?(key) @@ -42,17 +42,17 @@ class DataFileTest < ActiveSupport::TestCase test "#value_for returns matching value from given environment" do hierarchy = Hierarchy.find(@globs_environment, "Eyaml hierarchy") - data_file = DataFile.new(hierarchy: hierarchy, node: @node, path: @path) + data_file = DataFile.new(hierarchy:, node: @node, path: @path) key = Key.new(environment: @globs_environment, name: "hdm::float") - assert_equal "4.5", data_file.value_for(key: key).value + assert_equal "4.5", data_file.value_for(key:).value end test "#value_from_original_environment returns matching value from the node's original environment" do hierarchy = Hierarchy.find(@globs_environment, "Eyaml hierarchy") - data_file = DataFile.new(hierarchy: hierarchy, node: @node, path: @path) + data_file = DataFile.new(hierarchy:, node: @node, path: @path) key = Key.new(environment: @globs_environment, name: "hdm::float") - assert_equal "2.3", data_file.value_from_original_environment(key: key).value + assert_equal "2.3", data_file.value_from_original_environment(key:).value end end