Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 15 commits into from Dec 18, 2019

Conversation

arizvisa
Copy link
Contributor

@arizvisa arizvisa commented Feb 19, 2019

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.

Copy link
Contributor

@dwoz dwoz left a comment

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

@arizvisa
Copy link
Contributor Author

@arizvisa arizvisa commented Feb 20, 2019

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
Copy link
Contributor Author

@arizvisa arizvisa commented Feb 20, 2019

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
Copy link
Contributor Author

@arizvisa arizvisa commented Feb 20, 2019

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
Copy link
Contributor Author

@arizvisa arizvisa commented Feb 23, 2019

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
Copy link
Contributor Author

@arizvisa arizvisa commented Feb 23, 2019

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

@arizvisa
Copy link
Contributor Author

@arizvisa arizvisa commented Feb 25, 2019

(forced pushed due to rebase)

@arizvisa
Copy link
Contributor Author

@arizvisa arizvisa commented Feb 25, 2019

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
Copy link
Contributor Author

@arizvisa arizvisa commented Mar 8, 2019

Force-pushed due to rebase.

@arizvisa
Copy link
Contributor Author

@arizvisa arizvisa commented Apr 30, 2019

Any word on this?

@dwoz
Copy link
Contributor

@dwoz dwoz commented May 14, 2019

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

@arizvisa
Copy link
Contributor Author

@arizvisa arizvisa commented May 15, 2019

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
Copy link
Contributor Author

@arizvisa arizvisa commented Oct 3, 2019

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

@arizvisa arizvisa requested a review from as a code owner Oct 8, 2019
@ghost ghost requested a review from xeacott Oct 8, 2019
@arizvisa
Copy link
Contributor Author

@arizvisa arizvisa commented Oct 8, 2019

Re-based against master and force-pushed.

@arizvisa
Copy link
Contributor Author

@arizvisa arizvisa commented Oct 8, 2019

Umm...wtf?

@arizvisa arizvisa changed the base branch from develop to master Oct 8, 2019
@arizvisa
Copy link
Contributor Author

@arizvisa arizvisa commented Oct 8, 2019

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

@arizvisa
Copy link
Contributor Author

@arizvisa arizvisa commented Oct 16, 2019

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

@arizvisa
Copy link
Contributor Author

@arizvisa arizvisa commented Oct 16, 2019

@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
Copy link
Contributor Author

@arizvisa arizvisa commented Oct 23, 2019

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
Copy link
Contributor Author

@arizvisa arizvisa commented Dec 17, 2019

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
Copy link
Member

@s0undt3ch s0undt3ch commented Dec 17, 2019

By this I mean the pre-commit command

@s0undt3ch
Copy link
Member

@s0undt3ch s0undt3ch commented Dec 17, 2019

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

@arizvisa
Copy link
Contributor Author

@arizvisa arizvisa commented Dec 17, 2019

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

kk, gladly.

arizvisa added 15 commits Dec 17, 2019
… call the correct methods depending on Python2 vs Python3.
….utils.test_templates since nobody has done this.
… all of the files under requirements/static/py* with the hopes that the unit-tests pick them up.
…empy) to all of the files under requirements/static/py* with the hopes that the unit-tests pick them up."

This reverts commit 80bab27 as apparently it didn't work.
… the requirements.txt file with the hope that the unit-tests will pick them up.
…empy) to the requirements.txt file with the hope that the unit-tests will pick them up."

This reverts commit 00613c4 as apparently wempy's setup doesn't work properly on Python3.
… wempy) to the requirements/base.txt file with the hope that the unit-tests will pick them up properly.
…esulted in rendering with genshi for a test that should've been against wempy.
…ils.test_templates so it doesn't check for xml that wasn't defined in the template.
…i, Mako, wempy) to the requirements/base.txt file with the hope that the unit-tests will pick them up properly."

This reverts commit 43f9a0a.
… wempy) to the requirements/static/*.in files so that the unit-tests will pick them up properly (credit to @Ch3LL and @s0undt3ch).
@arizvisa
Copy link
Contributor Author

@arizvisa arizvisa commented Dec 17, 2019

(force push due to rebase)

@arizvisa
Copy link
Contributor Author

@arizvisa arizvisa commented Dec 17, 2019

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.

Copy link
Member

@s0undt3ch s0undt3ch left a comment

Thank You for putting up with me 😄

@s0undt3ch
Copy link
Member

@s0undt3ch s0undt3ch commented Dec 17, 2019

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

Nah, I'm good. Thank You.

Ch3LL
Ch3LL approved these changes Dec 18, 2019
@dwoz dwoz merged commit 78b84c5 into saltstack:master Dec 18, 2019
49 checks passed
@arizvisa
Copy link
Contributor Author

@arizvisa arizvisa commented Dec 18, 2019

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

@max-arnold
Copy link
Contributor

@max-arnold max-arnold commented Dec 20, 2019

@s0undt3ch
Copy link
Member

@s0undt3ch s0undt3ch commented Dec 20, 2019

No worries, #55712

arizvisa added a commit to arizvisa/lolfuzz3 that referenced this issue May 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants