search

saltstack-formulas / openssh-formula

http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
90 stars 297 forks source link

test(map): standardise `map.jinja` verification #193

Closed myii closed 3 years ago

myii commented 4 years ago

PR progress checklist (to be filled in by reviewers)

What type of PR is this?

Primary type

Secondary type

Does this PR introduce a BREAKING CHANGE?

No.

Related issues and/or pull requests

UPDATE: Automated using https://github.com/myii/ssf-formula/pull/281

~https://github.com/saltstack-formulas/sudoers-formula/pull/69~

Describe the changes you're proposing

Discussion in Slack: https://freenode.logbot.info/saltstack-formulas/20200826#c4886021-c4887283.

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Documentation checklist

Testing checklist

Additional context

pull-assistant[bot] commented 4 years ago
Score: 1.00

Best reviewed: commit by commit

Optimal code review plan

     test(_mapdata_spec): perform comparison using `describe yaml` instead

Powered by Pull Assistant. Last update 117443e ... 117443e. Read the comment docs.

myii commented 4 years ago

I made a test to see how it outputs diff with the following changes:

...

Not really easy for a human to spot the differences.

UPDATE: The diff situations has changed with the latest gems, see https://github.com/saltstack-formulas/openvpn-formula/pull/132#discussion_r509171592.

@baby-gnu So we've got a decision to make about whether we compare textually or using the YAML itself. This affects more than just this PR -- rather the way we do this across all of the formulas. The YAML method came about when I encountered problems doing this verification on Windows in the salt-formula. I had to resort to using gsub to match up the line endings (\r\n instead of \n). @dafyddj suggested matching by data structure and @daks provided the reference to matching the YAML. So trying to lay this out to help finalise what to use:

Factor Text (current) YAML (proposed)
InSpec diff output [+] Line-by-line, much easier to see the differences [-] Partial data structure, difficult to identify the differences
Windows [-] Hacky, have to resort to using gsub for \n => \r\n [+] Works in the same way as on Linux, no workarounds required
Comparison file ordering [?] Must be exact (alphabetical throughout) [?] As long as the data is correct, the ordering doesn’t matter

The last point ([?]) may actually work out as an advantage for the text-based method. By keeping all of the comparison files ordered alphabetically, working with/across the files becomes much easier. There have been times when you need to compare across the comparison files themselves, to see what changes (such as across releases such as ubuntu-16, ubuntu-18 & ubuntu-20).

So it seems that from a human perspective, using the text-based comparison actually works out better. The hack only has to be added in one place and that will probably end up controlled by the ssf-formula anyway.

dafyddj commented 3 years ago

Not sure where to put this, but I've seen applying this to Windows discussed somewhere and this is my attempt:

diff --git a/TEMPLATE/_mapdata/init.sls b/TEMPLATE/_mapdata/init.sls
index cfa4837..b998360 100644
--- a/TEMPLATE/_mapdata/init.sls
+++ b/TEMPLATE/_mapdata/init.sls
@@ -7,7 +7,8 @@

 {%- do salt['log.debug']('### MAP.JINJA DUMP ###\n' ~ mapdata | yaml(False)) %}

-{%- set output_file = '/tmp/salt_mapdata_dump.yaml' %}
+{%- set tmp = {'Windows': 'TMP'}[grains['kernel']] | default('TMPDIR') %}
+{%- set output_file = salt['environ.get'](tmp) ~ '/salt_mapdata_dump.yaml' %}

 {{ tplroot }}-mapdata-dump:
   file.managed:
diff --git a/test/integration/default/controls/_mapdata_spec.rb b/test/integration/default/controls/_mapdata_spec.rb
index 2e287f2..5ed4192 100644
--- a/test/integration/default/controls/_mapdata_spec.rb
+++ b/test/integration/default/controls/_mapdata_spec.rb
@@ -9,8 +9,25 @@ mapdata_dump = inspec.profile.file(mapdata_file)
 control '`map.jinja` YAML dump' do
   title 'should contain the lines'

-  describe file('/tmp/salt_mapdata_dump.yaml') do
+  tmp =
+    case platform[:family]
+    when 'windows'
+      'TMP'
+    else
+      'TMPDIR'
+    end
+
+  mapdata_file = file("#{os_env(tmp).content}/salt_mapdata_dump.yaml")
+
+  describe mapdata_file do
     it { should exist }
-    its('content') { should eq mapdata_dump }
+  end
+
+  mapdata_content = mapdata_file.content.encode(universal_newline: true)
+
+  describe 'File content' do
+    it 'should match profile map data exactly' do
+      expect(mapdata_content).to eq(mapdata_dump)
+    end
   end
 end
baby-gnu commented 3 years ago

Not sure where to put this, but I've seen applying this to Windows discussed somewhere and this is my attempt:

Thanks @dafyddj, there is no TMP or TMPDIR in my GNU/Linux minion environments so it should a little more complicated to support both.

I would more something like:

diff --git a/openssh/_mapdata/init.sls b/openssh/_mapdata/init.sls
index 5e4fcf1..46cf87f 100644
--- a/openssh/_mapdata/init.sls
+++ b/openssh/_mapdata/init.sls
@@ -5,7 +5,13 @@
 {%- set tplroot = tpldir.split('/')[0] %}
 {%- from tplroot ~ "/map.jinja" import mapdata with context %}

-{%- set output_file = '/tmp/salt_mapdata_dump.yaml' %}
+{%- if salt["grains.get"]("kernel") == 'Windows' %}
+{%-   set tmp_dir = salt["environ.get"]("TMP") %}
+{%- else %}
+{%-   set tmp_dir = '/tmp' %}
+{%- endif %}
+
+{%- set output_file = tmp_dir ~ '/salt_mapdata_dump.yaml' %}

 {%- do salt['log.debug']( mapdata | yaml(False) ) %}

But thanks a lot for the mapdata_file.content.encode(universal_newline: true) I was not aware of it ;-)

myii commented 3 years ago

@dafyddj The strange thing about this path issue is that it actually worked as-is during my Windows testing in the salt-formula a couple of months ago:

https://github.com/myii/salt-formula/runs/1032273456?check_suite_focus=true#step:8:1532

      [PASS]  File /tmp/salt_mapdata_dump.yaml content is expected to eq "# yamllint disable rule:indentation rule:line-length\r\n# Windows-2019Server\r\n---\r\nformulas_sett...yndic\r\n  ssh_roster: {}\r\n  syndic_service: salt-syndic\r\n  use_pip: false\r\n  version: ''\r\n"
{%- set output_file = '/tmp/salt_mapdata_dump.yaml' %}
+{%- if salt["grains.get"]("kernel") == 'Windows' %}
+{%-   set tmp_dir = salt["environ.get"]("TMP") %}
+{%- else %}
+{%-   set tmp_dir = '/tmp' %}
+{%- endif %}

@baby-gnu Out of interest, is there any specific benefit to using grains.kernel over grains.os_family (which is also Windows)?

baby-gnu commented 3 years ago

@dafyddj The strange thing about this path issue is that it actually worked as-is during my Windows testing in the salt-formula a couple of months ago:

I'll recheck because on my windows systems there is no c:\tmp. Maybe it came with some tooling installed for the CI? I only try to run the formula on a pristine windows 10 machine + salt-minion.

@baby-gnu Out of interest, is there any specific benefit to using grains.kernel over grains.os_family (which is also Windows)?

In that specific case, absolutely no benefit, I used grains["kernel"] somewhere else to distinguish between windows and all linux systems. It was in my mind and my fingers type that without much conscious thinking ;-)

NB: sorry, I edited your comment instead of quote reply, I think I need a nap ;-)

myii commented 3 years ago

I've updated my comment above (https://github.com/saltstack-formulas/openssh-formula/pull/193#issuecomment-682430080) because the diff is no longer showing when doing a text-based comparison using eq.

myii commented 3 years ago

Repurposed this PR with all of the standardisations worked out across the various formulas.

myii commented 3 years ago

Self-merging since tests only.

saltstack-formulas-travis commented 3 years ago

:tada: This PR is included in version 2.0.5 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: