Release notes for v3000 do not mention changes to slspath variable

[finalduty]

Description of Issue

Release notes for v3000 do not mention changes to slspath variable by #55434 / #55433 , which previously returned the parent directory instead of the path to the state file itself.

This functionality appears to be covered by the tpldir variable, but this is not documented on https://docs.saltstack.com/en/latest/ref/states/vars.html.

Can both the v3000 release notes and the SLS Template Variable Reference be updated to make this more visible to others?

Setup

Tree:

/srv/salt/users/
|-- files
|   |-- bash_profile
|   |-- bashrc
|   |-- login.defs
|   |-- root_profile
|   `-- vimrc
|-- init.sls
|-- root.sls
`-- staff.sls

root.sls

/root/.bashrc:
  file.managed:
    - source: salt://{{ slspath }}/files/bashrc
    - template: jinja
    - defaults:
        sls: {{ sls }}

Steps to Reproduce Issue

1. Apply users.root state

Output:

----------
          ID: /root/.bashrc
    Function: file.managed
      Result: False
     Comment: Source file salt://users/root/files/bashrc not found in saltenv 'base'
     Started: 20:46:08.752499
    Duration: 2.315 ms
     Changes:   
----------

Replacing slspath with tpldir resolves the issue, but this is not an obvious fix.

Versions Report

Salt Version:
           Salt: 3000
 
Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: Not Installed
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
         Jinja2: 2.8.1
        libgit2: Not Installed
       M2Crypto: 0.33.0
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.6.2
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: 3.7.3
         pygit2: Not Installed
         Python: 3.6.8 (default, Aug  7 2019, 17:28:10)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 15.3.0
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.1.4
 
System Versions:
           dist: centos 7.7.1908 Core
         locale: ANSI_X3.4-1968
        machine: x86_64
        release: 3.10.0-1062.9.1.el7.x86_64
         system: Linux
        version: CentOS Linux 7.7.1908 Core

[ryanmilne-digicert]

+1

I was about to create an issue for the exact same problem. Good find with the tplpath variable.

[tomwalsh]

Add me a +1. This breaks previously working SLS files and it is frustrating to have this happen with ZERO indication.

[PaulChristophel]

+1. Sure do love these breaking changes.

[OrangeDog]

For tpldir not being documented: #50925

[max-arnold]

A quick patch:

patches/slspath.sls:

{% if grains.saltversion == "3000" %}
patch:
  pkg.installed

slspath_patch:
  file.patch:
    - name: '{{ grains.saltpath }}'
    - source: salt://patches/files/3000-slspath.diff
    - strip: 2
    - require:
        - pkg: patch

restart_salt_minion:
  cmd.run:
    - name: 'salt-call service.restart salt-minion'
    - bg: true
    - onchanges:
      - file: slspath_patch
{%- endif %}

patches/files/3000-slspath.diff:

diff --git a/salt/utils/templates.py b/salt/utils/templates.py
index d026118269..98092a9d79 100644
--- a/salt/utils/templates.py
+++ b/salt/utils/templates.py
@@ -122,6 +122,8 @@ def wrap_tmpl_func(render_str):
             slspath = context['sls'].replace('.', '/')
             if tmplpath is not None:
                 context['tplpath'] = tmplpath
+                if not tmplpath.lower().replace('\\', '/').endswith('/init.sls'):
+                    slspath = os.path.dirname(slspath)
                 template = tmplpath.replace('\\', '/')
                 i = template.rfind(slspath.replace('.', '/'))
                 if i != -1:

Run sudo salt MINION state.apply patches.slspath to apply the fix.

[danlsgiga]

Just hit this wall too and changed all my {{ slspath }} code blocks for the undocumented {{ tpldir }} one.

[commonism]

The commit:
5a17dd6#diff-96d7010eb773dc57ad2c8eb89cd12807

As all (relevant) unittests reported success, it may be a good idea to harden the variables with unittests.

[brandon-sling]

This is tangential but I was wondering why slspath and tpldir were the same in previous versions. I have almost filed an issue in the past to see if slspath could be updated to do what it does now, and document tpldir

[dkacar-oradian]

I don't understand how is this going to be resolved. Is the change to slspath value going to become permanent (in which case I need to replace slspath variable with tpldir everywhere) or it's going to be reverted to the previous value (in which case I just need to wait for the next salt release)?

[dhs-rec]

Since this has gone completely undocumented AND is a backwards incompatible change, I'd vote for reverting it to the previous behaviour and only then announce it as being deprecated and change it in the second next release. Nothing else does make any sense.

[sagetherage]

Looking into this @dkacar-oradian @dhs-rec we will add this to the this week's Community Open Hour on Thursday at 11 AM Mountain and get more details in this ticket. Apologies! I didn't see this early, Monday was a US holiday and simply catching up, not to defend or give you and excuse, we need to hear from you and deal with this ASAP, simply giving context for the lack of a more prompt response.

[dwoz]

The change needs to be reverted and go through a proper deprecation path.

[terminalmage]

That change should absolutely not be reverted. It was a bugfix.

From the documentation:

The slspath variable contains the path to the current sls file. The value of slspath
in files referenced in the current sls depends on the reference method. For jinja
includes slspath is the path to the current file. For salt includes slspath is the path
to the included file.

"the path to the current SLS file". Not a directory.

[dhs-rec]

Yeah, but the point is: People are using this, now, in the way it was implemented, not the way it was documented. Changing the behaviour now, regardless of being a bugfix or a feature, breaks those people's code. Period.

[OrangeDog]

slspath should be fixed, but not as an undocumented breaking change. I wouldn't even object to it being fixed without a deprecation period, because everyone should've been using tpldir instead. But you have to actually document all of this.

[terminalmage]

Yeah, I agree. The correct move here is to build out the documentation, not to break slspath again.

EDIT: I've changed my mind, see below.

[terminalmage]

@dhs-rec And some people are expecting slspath to work as documented. Before, tpldir and slspath did the same thing, and that's clearly not the intent. In addition, it deprives people of a means to get the path of the SLS file.

[terminalmage]

After looking at the code again, the bugfix didn't even restore the documented behavior, since the path to the SLS file has already at that point had its .sls removed (making it foo/bar instead of foo/bar.sls when {{ slspath }} is referenced within salt://foo/bar.sls).

Given that swing-and-miss on the bugfix, it's probably better to proceed with reverting in #56341, provided that the following is also added to that PR:

1. Document that tpldir and slspath both refer to the same path. Before, they just ended up the same 100% of the time due to the os.path.dirname() only being used when the basename of the path isn't init.sls. Rather than letting this happy accident remain in the code, slspath should probably just be assigned the value of tpldir to prevent them from possibly drifting from one another in the future.
2. Create a new SLS variable (perhaps named slsfile) which has the actual value of the path relative to salt://, and make sure that this new variable is documented.

[OrangeDog]

the bugfix didn't even restore the documented behavior

Perhaps GitHub should add 🤦 as a reaction option.

Yeah, I agree.

@terminalmage with what? Is "change should absolutely not be reverted" no longer your position? Perhaps you should edit or hide that comment for clarity.

Create a new SLS variable (perhaps named slsfile)

In which case I think slspath should be deprecated and then fully removed, due to its misleading name. I would still prefer that slspath be actually fixed, with sufficient communication of the change.

[terminalmage]

These are all excellent points (especially the one regarding the proposed new reaction type 😄), and I went back and edited my earlier comment.

[OrangeDog]

Another observation - tpldir as I understand supposed to work in all templates, not just .sls files. Would it be more confusing if slsplath/slsfile also did?

In e.g. a map.jinja should slspath/slsfile instead refer to the path of the .sls that is including the template?

Update: if I'm reading the comment linked below correctly - tpl* and sls* behave(d) the same in all templates, and don't work properly in non-sls files.

[max-arnold]

FYI: I did some research on how various Jinja variables behave inside of sls and map.jinja files: #41195 (comment)

[dwoz]

Another thing I noticed about this change is it made things in-consistent. With the change, running state with the foo/init.sls would return still return the directory where the init.sls is located foo. Running a state within a directory foo/bat.sls wougld return foo/bat.

When the change is reverted both of the above scenarios return foo. Overall, I think the name slspath is very misleading and should be addressed. Perhaps by deprecating it all together and making slsdir instead. I could also see a case for adding slsdir and changing slspath to reflect the actual path (including the file extension).

In any case, for the 3000.1 release, I have reverted the change and updated the documentation for slspath to more accurately reflect the behavior.

[Ch3LL]

closing since resolved in 3000.1 with #56341

seems like there might be some discussion that could occur in a SEP if we want to change and properly deprecate things here.

[mlasevich]

For those who care about this - be careful switching to tpldir - tpldir is currently buggy and does not always work as you expect. I just filed a bug about this (#56410 )