Fix the Cheetah template renderer in salt.utils.templates, and unit-tests for Jinja, Cheetah, Mako, Genshi, wempy templates,

[arizvisa]

What does this PR do?

The author of the Cheetah template in salt.utils.templates forgot to explicitly call str() or unicode() on the template in order to render it. This PR checks the tmplstr parameter and uses it to distinguish which method to call so that templates using the Cheetah engine will work.

What issues does this PR fix or reference?

This fixes issue #51711.

Previous Behavior

The Cheetah template engine did not work and would result in an exception due to the template not returning a string type.

New Behavior

Now the Cheetah template engine works and renders properly.

Tests written?

No.

Commits signed with GPG?

No.

[dwoz]

@arizvisa Since this is a bug fix, can you please write a regression test? Also, There is a warning from the linter. Thanks. :)

[arizvisa]

Wrt this lint text (python3 incompat), how does one distinguish between calling obj.__unicode__ and obj.__str__?

The issue is that if the text is unicode, then .__unicode__() needs to be called to render unicode, and .__str__() needs to be called to render bytes? Do I just explicitly call those methods instead of "casting" to the specified type?

[arizvisa]

Also, how do I write a regression test since it had never worked before (for 1 year at least)? Plus nobody has written tests for anything but Jinja? Do any of these other templates even work?

[arizvisa]

Okay, good the lint test passed. I explicitly tested the Python version and explicitly called .__str__() versus .__unicode__() so that (hopefully) the right renderer for Cheetah gets called.

It seems hacky due to having to call private methods, but I'm not sure of a cleaner way to accomplish this since .__unicode__() doesn't exist in Python3, yet Cheetah still uses it regardless. (See prior question).

[arizvisa]

Any word on the questions for this too? Technically it's a new feature since the Cheetah template engine hasn't worked since salt.utils.data.decode stopped explicitly calling str() which has existed this way for at least a year(?).

[arizvisa]

Some of these tests need to be punted as the junkins one failed only because it couldn't connect to y'alls slack.

[arizvisa]

(forced pushed due to rebase)

[arizvisa]

Hmm..why is salt.utils.is_running not being loaded in the tests for this? I walked through the backtrace and nothing there is importing my module, and I tested via the following (which is what the backtrace is doing) and it works without a problem?

$ salt --async \* state.apply
Executed command with job ID: 20190225210028096476
$ salt \* saltutil.is_running 'state.*'
888e0dab3011409888f36dbf6153f9cf:
    |_
      ----------
      arg:
      fun:
          state.apply
      jid:
          20190225210028096476
      master_id:
          888e0dab3011409888f36dbf6153f9cf
      pid:
          508
      ret:
          etcd
      tgt:
          *
      tgt_type:
          glob
      user:
          root
bX_nxPEVRyWOcIDBECwW:
    |_
      ----------
      arg:
      fun:
          state.apply
      jid:
          20190225210028096476
      master_id:
          888e0dab3011409888f36dbf6153f9cf
      pid:
          2136
      ret:
          etcd
      tgt:
          *
      tgt_type:
          glob
      user:
          root


-------------------------------------------
Summary
-------------------------------------------
# of minions targeted: 2
# of minions returned: 2
# of minions that did not return: 0
# of minions with errors: 0
-------------------------------------------
$

[arizvisa]

Force-pushed due to rebase.

[arizvisa]

Any word on this?

[dwoz]

@arizvisa Are you able to add a test that covers this change?

[arizvisa]

Nah, there aren't any tests for any of the other template renderers (other than Jinja) to base it off of. This is why Cheetah hasn't been detected as non-working until using it.

I don't do engineering as my primary job and so I don't have any way of justifying development on this to the company I work for. If there were testcases for any of the other renderers (other than Jinja), then I could do a search+replace for it, though.

The patch is simple, however, and so essentially the test could be just a smoke-test which should've been implemented when the feature was added anyways.

[arizvisa]

Are you guys using the "Needs Testcase" label yet?

[arizvisa]

Re-based against master and force-pushed.

[arizvisa]

Umm...wtf?

[arizvisa]

Hah. okay. Updated PR to target the master branch.

[arizvisa]

Well that was fun learning the syntax of all the available template types...
Hopefully the wempy tests check out, heh.

[arizvisa]

@dwoz, @xeacott: Hey, good sirs.

For the unit tests in this PR. It seems as if I need to import the related template modules into the test system since they haven't had any tests written for them before. I looked around in the documentation, but didn't see any references on how to properly do this.

Is there some place to add them under tests/integration, or do I need to add them to one of the kitchen-related files to get them to be installed for testing?

(edited to not end the last sentence with a preposition)

[arizvisa]

Hey @Ch3LL, can you help me out with my question? I'm not sure what's the proper way to update kitchen or tests/integration to add the required modules for the testcases that i wrote for this PR.

[arizvisa]

Did you manily editted the *.txt files?
What python versions do you locally have available?
What python version did you install pre-commit into?
And finnaly, what platform did you run this on?
Sorry for all the questions but it will shed some lights on some doubts I'm having with the generated requirements files.

Nah, I didn't. The first commit is the revert of the original modification of requirements.txt, the second commit is the modification of the .in files, the third commit was for the execution of pre-commit.

(testkit) [1121] user@E5570 ~/work/salt$ python -V
Python 3.7.5

I have a couple versions of Python, but this is using a virtualenv and my PATH definitely pointing to it. This is on a fedora box (fc31), logs of everything that was done to make those commits are at https://gist.github.com/arizvisa/94b51853762b41a74d4479caa1460788

[s0undt3ch]

By this I mean the pre-commit command

[s0undt3ch]

I see that you now also have conflicts so you will also need to rebase.

[arizvisa]

I see that you now also have conflicts so you will also need to rebase.

kk, gladly.

[arizvisa]

(force push due to rebase)

[arizvisa]

Do you want a freeze of the requirements from the virtualenv in a gist? Its an old virtualenv so its likely not using the most recent of anything else other than pre-commit.

[s0undt3ch]

Thank You for putting up with me 😄

[s0undt3ch]

Do you want a freeze of the requirements from the virtualenv in a gist?

Nah, I'm good. Thank You.

[arizvisa]

Thanks guys. Now for my etcd returner PR, heh.

[max-arnold]

@s0undt3ch Double pyobjc requirement has sneaked in:

https://github.com/saltstack/salt/blob/master/requirements/static/py2.7/darwin.txt#L135-L138

[s0undt3ch]

No worries, #55712