saltstack-formulas / users-formula

Configure users via pillar
http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
99 stars 362 forks source link

[BUG] vimrc state keeps not working #213

Closed fzipi closed 4 years ago

fzipi commented 4 years ago

Your setup

Formula commit hash / release tag

Using 9ee7636477e20ad6597da2dd41375e858f644e4d.

Versions reports (master & minion)

I'm just using kitchen test default-ubuntu-1804-2019-2-py3

Pillar / config used

Testing the pillar with the example test (test/salt/pillar/default.sls), and small patch. See below.


Bug details

Describe the bug

After reading #166 , we still cannot make this state to work. I'm getting always

       salt.exceptions.SaltRenderError: Jinja variable 'dict object' has no attribute 'use_vim_formula'
       [CRITICAL] Rendering SLS 'base:users.vimrc' failed: Jinja variable 'dict object' has no attribute 'use_vim_formula'
       [DEBUG   ] LazyLoaded highstate.output
       local:
           Data failed to compile:
       ----------
           Rendering SLS 'base:users.vimrc' failed: Jinja variable 'dict object' has no attribute 'use_vim_formula'

Steps to reproduce the bug

I'm using this patch in master to test:

diff --git a/kitchen.yml b/kitchen.yml
index 69d8b54..4b72387 100644
--- a/kitchen.yml
+++ b/kitchen.yml
@@ -180,6 +180,10 @@ provisioner:
   salt_install: none
   require_chef: false
   formula: users
+  dependencies:
+  - name: vim
+    repo: git
+    source: https://github.com/saltstack-formulas/vim-formula.git
   salt_copy_filter:
     - .kitchen
     - .git
@@ -198,12 +202,23 @@ suites:
       state_top:
         base:
           '*':
+            - vim
             - users
+            - users.vimrc
       pillars:
         top.sls:
           base:
             '*':
+              - vim
               - users
+        vim.sls:
+          vim:
+            managed_vimrc: true
+            allow_localrc: false
+            config:
+              syntax: 'on'
+              colors: desert
+
       pillars_from_files:
         users.sls: test/salt/pillar/default.sls
     verifier:
diff --git a/test/integration/default/controls/config_spec.rb b/test/integration/default/controls/config_spec.rb
index 6af3cf9..0f28157 100644
--- a/test/integration/default/controls/config_spec.rb
+++ b/test/integration/default/controls/config_spec.rb
@@ -10,3 +10,13 @@ control 'users configuration' do
     its('mode') { should cmp '0750' }
   end
 end
+
+control 'vim formula works' do
+  title 'formula should createfile'
+
+  describe file('/home/auser/.vimrc') do
+    it { should be_owned_by 'auser' }
+    its('mode') { should cmp '0644' }
+    its('content') { should match /syntax on/ }
+  end
+end
diff --git a/test/salt/pillar/default.sls b/test/salt/pillar/default.sls
index 0bf7025..23da8d8 100644
--- a/test/salt/pillar/default.sls
+++ b/test/salt/pillar/default.sls
@@ -33,6 +33,7 @@ users:
   ## Minimal required pillar values
   auser:
     fullname: A User
+    manage_vimrc: true

   ## Full list of pillar values
   buser:

Expected behaviour

A /home/auser/.vimrc file should exist.

Attempts to fix the bug

Well, many. There seems to be a difference between documentation in docs/README.rst: This depends on the vim-formula being available and pillar \users:use_vim_formula: True`.vs. usingusers-formula: use_vim_formula: true`.

If I remove/change {% if users.use_vim_formula %} in users/vimrc.sls, of course this error is not there anymore. But then, it never gets executed also.

Additional context

myii commented 4 years ago

@fzipi Thanks for the report, just some initial feedback for you. While we can look into why users.vimrc isn't working, you may find users.user_files more useful overall. You can use it to manage .vimrc and any other files as well. This is the relevant section in pillar.example:

https://github.com/saltstack-formulas/users-formula/blob/1d9a5ef5be4bf0c66d6471effa32a2953637b031/pillar.example#L170-L186

fzipi commented 4 years ago

I saw that, seems good. Just trying to be compatible with our old pillars (we were using an ancient users-formula version).

Do you think the user_files is going to be the way to do it in the future? (makes sense if you want to be independent of the vim formula)

myii commented 4 years ago

@fzipi I would say that user_files is a better solution all round, compared to users.vimrc specifically. In the meantime, I didn't see this in your original report:

https://github.com/saltstack-formulas/users-formula/blob/1d9a5ef5be4bf0c66d6471effa32a2953637b031/pillar.example#L4-L5

Note, this is using the users-formula pillar key as opposed to users. Do you have that defined somewhere in your setup?

fzipi commented 4 years ago

@myii Well, it is your setup. It is using the kitchen test files :)

I have it also in my setup ;)

myii commented 4 years ago

@fzipi Well, we're not not currently testing the users.vimrc state at the current time!

https://github.com/saltstack-formulas/users-formula/blob/1d9a5ef5be4bf0c66d6471effa32a2953637b031/kitchen.yml#L179-L182

This is something that would be worth adding, though.

fzipi commented 4 years ago

@myii That's what kind of added with my patch. To test that it is not working, sadly.

If you apply the patch, you'll see that it is indeed not working.

myii commented 4 years ago

@fzipi I'm having a little look at this, I'll get back to you soon with some feedback. I see the contradiction that was introduced in #177, as you linked above.

myii commented 4 years ago

@fzipi OK, progress made, got the state running but failing:

The changes made so far (ignore .travis.yml, that's just to prevent all of the instances from running):

Removing jinja as the template will probably finalise this.

fzipi commented 4 years ago

Ok, this quick patch makes sense, and it is more or less what I've done:

{% if salt['pillar.get']('users-formula:use_vim_formula', False) %}
myii commented 4 years ago

@fzipi It's working now:

Using no template:

fzipi commented 4 years ago

Ok, now the question is is it really working? I mean, I've added some tests to see if the file ~/.vimrc is created. Is that what this state is supposed to do?

fzipi commented 4 years ago

Oh, yes. I see it now in my local test :). Thanks!

myii commented 4 years ago

@fzipi You're welcome, thanks for the detailed bug report in the first place. It makes it much easier to conduct the bug hunt.

Moving forward, the obvious thing is to add these changes to the formula. There is a breaking change that needs to be avoided if possible, though: there may be users out there who are using .vimrc Jinja templates. So may need to include the option of selecting the template as well, although I'm not keen on that either.

fzipi commented 4 years ago

@myii For me the question is in this case if this was woking at all.... I don't see how after introducing the users.use_vim_formula could have worked properly...

But I got your point.

myii commented 4 years ago

@fzipi That most likely happened during the review of #177, specifically:

So this line also needed to be changed after the review but got overlooked.

fzipi commented 4 years ago

Maybe relying on the extension? Like if file ends in .jinja then apply jinja templating or nothing if not.

myii commented 4 years ago

That won't work as-is, since the source has been configured to look for vimrc without an extension:

https://github.com/saltstack-formulas/users-formula/blob/1d9a5ef5be4bf0c66d6471effa32a2953637b031/users/vimrc.sls#L29-L31

So the least harmful solution appears to be to modify users/files/vimrc/vimrc to be an actual jinja template.

fzipi commented 4 years ago

Well, reading the file, we can think about just keeping things like they are now, and removing the {{{1 from the file, so they don't get parsed as jinja template tags....

myii commented 4 years ago

Actually, I've already done this when managing files with user_files. We can simply wrap most/all of the vimrc template in raw tags:

{% raw -%}
...
{%- endraw %}

That will solve this problem, without causing a breaking change. Just going to test that now.

fzipi commented 4 years ago

Oh, that sounds like a lovely solution!

myii commented 4 years ago

There we are, that's working:

I'll wrap this all into a PR so we can get this out there. Thanks again for all of the feedback, @fzipi!

fzipi commented 4 years ago

Thanks to you!

fzipi commented 4 years ago

If you want to add it to the PR, I've added some inspec test in the patch to verify that .vimrc is being created properly.

myii commented 4 years ago

@fzipi Sure, I was just about to work on this! Do you mind putting this into a PR? We can merge this fix first and then the InSpec tests.

fzipi commented 4 years ago

Created https://github.com/saltstack-formulas/users-formula/pull/214.

myii commented 4 years ago

@fzipi Thanks, I'll continue the discussion there.

saltstack-formulas-travis commented 4 years ago

:tada: This issue has been resolved in version 0.48.4 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: