Skip to content

Conversation

@baby-gnu
Copy link
Contributor

No description provided.

@baby-gnu baby-gnu requested review from alxwr and myii July 23, 2019 08:54
@baby-gnu
Copy link
Contributor Author

baby-gnu commented Jul 23, 2019

This should fix #149.

This is the output on my test machine:

       template-config-file-file-managed:
         file.managed:
           - name: /etc/template-formula.conf
           - source: 
             - salt://template/files/any/path/can/be/used/here/example.tmpl.jinja
             - salt://template/files/e4585afd55d3/example.tmpl.jinja
             - salt://template/files/Debian-9/example.tmpl.jinja
             - salt://template/files/Debian/example.tmpl.jinja
             - salt://template/files/Debian/example.tmpl.jinja
             - salt://template/files/foo/example.tmpl.jinja
             - salt://template/files/bar/example.tmpl.jinja
             - salt://template/files/default/example.tmpl.jinja

The two lines before the default come from the role.

I'm not really happy with putting the role directly in pillar.example, shouldn't we have a dedicated pillar for tests? Maybe even using both of them?

@baby-gnu baby-gnu force-pushed the bugfix/libtofs-crash-on-files_switch-lookup-list branch from 0c9eadd to ee0dad6 Compare July 23, 2019 09:06
@baby-gnu
Copy link
Contributor Author

I'm not really happy with putting the role directly in pillar.example, shouldn't we have a dedicated pillar for tests? Maybe even using both of them?

Maybe something like

diff --git a/kitchen.yml b/kitchen.yml
index 3be550a..d62989d 100644
--- a/kitchen.yml
+++ b/kitchen.yml
@@ -124,8 +124,10 @@ provisioner:
       base:
         '*':
           - template
+          - test
   pillars_from_files:
     template.sls: pillar.example
+    test.sls: test/salt/pillar/test.sls
 
 verifier:
   # https://www.inspec.io/
diff --git a/pillar.example b/pillar.example
index fd8a5b8..1b035ec 100644
--- a/pillar.example
+++ b/pillar.example
@@ -56,7 +56,3 @@ template:
   # Just for testing purposes
   winner: pillar
   added_in_pillar: pillar_value
-
-role:
-  - foo
-  - bar
diff --git a/test/salt/pillar/test.sls b/test/salt/pillar/test.sls
new file mode 100644
index 0000000..5ac428c
--- /dev/null
+++ b/test/salt/pillar/test.sls
@@ -0,0 +1,4 @@
+# libtofs.jinja must work with tofs.files_switch looked up list
+role:
+  - foo
+  - bar

@myii
Copy link
Contributor

myii commented Jul 23, 2019

@baby-gnu That's a prompt response! Thanks for running with this, I'll check it out as soon as I get a chance.

@ErisDS You may be interested in this, since this was instigated by our discussion on Slack yesterday.

@myii
Copy link
Contributor

myii commented Jul 23, 2019

@baby-gnu Using an extra pillar for testing is definitely a solution we can work with. I've already adapted ssf-formula to deal with this, so fine at my end.

@baby-gnu
Copy link
Contributor Author

Using an extra pillar for testing is definitely a solution we can work with. I've already adapted ssf-formula to deal with this, so fine at my end.

Thanks @myii, I'll integrate this.

Where is the ssf-formula to have a look?

@myii
Copy link
Contributor

myii commented Jul 23, 2019

@baby-gnu I'll be sharing it soon! See saltstack-formulas/cert-formula#28 (comment) for a bit more explanation.

@baby-gnu baby-gnu force-pushed the bugfix/libtofs-crash-on-files_switch-lookup-list branch 2 times, most recently from 2e6110b to 3eb4ff5 Compare July 23, 2019 11:46
Copy link
Contributor

@myii myii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@baby-gnu Fantastic, this is the same method that was in my mind. Most of my review is just a number of suggestions; the only thing I'd really like to change is role => roles, for the reason I mentioned inline (re: the upstream documentation).

@myii
Copy link
Contributor

myii commented Jul 23, 2019

@baby-gnu By the way, I forgot to mention that I tested my suggested changes and all is looking good:

Furthermore, if you do agree to change to roles instead, I'm sorry for having to get you to modify both/all of your commits!

@baby-gnu baby-gnu force-pushed the bugfix/libtofs-crash-on-files_switch-lookup-list branch from 3eb4ff5 to 3aaf5b9 Compare July 23, 2019 14:18
baby-gnu added 2 commits July 24, 2019 12:13
We need to verify that “libtofs” works when the “tofs.files_switch”
lookup return a list instead of a single element.

* kitchen.yml (provisioner): copy tests specific pillars into the test
  environment.

* pillar.example (template.tofs.files_switch): lookup for “roles” at
  the end.

* test/salt/pillar/test.sls (roles): list of roles to apply during
  tests.

* test/integration/default/controls/config_spec.rb: add “roles” to the
  list of “tofs.files_switch” items to verify.
We need to process all elements when the “config.get” lookup of
“tofs.files_switch” return a list.

* template/libtofs.jinja: force the output of the lookup to be a list.
@baby-gnu baby-gnu force-pushed the bugfix/libtofs-crash-on-files_switch-lookup-list branch from 3aaf5b9 to 0979d35 Compare July 24, 2019 10:14
@baby-gnu
Copy link
Contributor Author

I just fixed the commit messages roleroles.

Thanks @myii for the note in IRC.

@myii myii merged commit 4b2d9e4 into saltstack-formulas:develop Jul 24, 2019
@myii
Copy link
Contributor

myii commented Jul 24, 2019

Merged, thanks for the important contribution @baby-gnu!

@baby-gnu baby-gnu deleted the bugfix/libtofs-crash-on-files_switch-lookup-list branch July 24, 2019 11:57
@saltstack-formulas-travis

🎉 This PR is included in version 3.0.9 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants