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

jinja renderer error reporting not fixed #13408

Closed
jberkus opened this issue Jun 11, 2014 · 9 comments · Fixed by #13416
Closed

jinja renderer error reporting not fixed #13408

jberkus opened this issue Jun 11, 2014 · 9 comments · Fixed by #13416
Labels
Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Milestone

Comments

@jberkus
Copy link

jberkus commented Jun 11, 2014

Per issue #8418, Salt was misreporting the line number of jinja renderer errors. This was supposedly fixed with pull #8475 in 2014.1.0. However, I just encountered the same error in 2014.1.4, indicating that the underlying issue is not, in fact, fixed. Is this something wrong with jinja?

Error example. As you can see, jinja is reporting an error with the variable hr_db, but pointing to a line containing a different variable (a line which is about 30 lines away from the actual source of the bug).

 Comment: Unable to manage file: Jinja variable 'hr_db' is undefined; line 83

          ---
          [...]
                  role = master
                  failover_priority = 1
                  enabled = True

              [[{{ repdb }}]]
                  hostname = {{ repdb }}    <======================
                  role = replica
                  failover_priority = 2
                  enabled = False

          [...]
          ---
@gtmanfred
Copy link
Contributor

Can you provide the whole file for reference?

@jberkus
Copy link
Author

jberkus commented Jun 11, 2014

FWIW, I have no idea how it picked that line or that variable. It's not the next variable reference in the file, and it's not the next one per set statement.

@jberkus
Copy link
Author

jberkus commented Jun 11, 2014

I'll have to sanitize it. Give me a bit.

@jberkus
Copy link
Author

jberkus commented Jun 11, 2014

[root@salt01 salt]# salt --version
salt 2014.1.4

I need to get the owner's permission to paste the jinja file as a gist. I can email it, or I can get their permisison later.

@jberkus
Copy link
Author

jberkus commented Jun 12, 2014

Here: http://salt.privatepaste.com/9a776ed727

That paste will expire in a week. The owner won't allow me to post it to a permanent searchable location.

@terminalmage
Copy link
Contributor

I've confirmed this, both on develop and on 2014.1.4. I've marked this as a regression, but it is possible that my fix a few months ago was just incomplete. Taking a deeper look now.

@terminalmage
Copy link
Contributor

OK, it's definitely not a regression, it's just an incomplete fix. The problem is a jinja bug. The traceback is reporting the wrong line number for the error. Here is the raw exception stack from the Jinja exception:

[('/home/erik/salt/salt/utils/templates.py',
  275,
  'render_jinja_tmpl',
  'output = template.render(**unicode_context)'),
 ('/usr/lib/python2.6/site-packages/jinja2/environment.py',
  969,
  'render',
  'return self.environment.handle_exception(exc_info, True)'),
 ('/usr/lib/python2.6/site-packages/jinja2/environment.py',
  742,
  'handle_exception',
  'reraise(exc_type, exc_value, tb)'),
 ('<template>', 82, 'top-level template code', None)]

As you can see, Jinja is reporting that the error in the template is on line 82. Note that this line number differs slightly from yours because I slightly edited the pastebin that was posted, as I don't have the pillar variables you have.

This is a known bug in Jinja, that I happened to have reported about 7 months ago (pallets/jinja#276), though it appears that not a single bit of work has been done to even replicate it, let alone fix it.

Until this gets fixed (if it ever gets fixed), the only realistic solution is not to report the line number for undefined jinja variables, which would simply leave the comment as Unable to manage file: Jinja variable 'hr_db' is undefined. It's not the best solution, but it's better than reporting an incorrect line.

@jberkus
Copy link
Author

jberkus commented Jun 12, 2014

Yeah, I suspected an underlying Jinja2 error. Maybe Salt and Django need to get together and fork the project so it'll be maintained?

For my part, not reporting an incorrect line would be an improvement over reporting a wrong one. Is it wrong most of the time though?

@terminalmage
Copy link
Contributor

For UndefinedError exceptions, the reported line is usually wrong if there is more than one variable expansion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants