koji

#4346 port kojiweb to jinja2
Merged by tkopecek. Opened by mikem.
mikem/koji jinja2  into  master

Download 4346.patch

This almost removes the Cheetah dependency from Koji. We still need it on builders for backwards compatibility with wrapper rpms (~~though I have added the option of using jinja templates for those as well~~)

My aim was to avoid rewriting the templates entirely. For the most part they have the same structure, with syntax adapted to work with jinja2. I also made use of the line_statements option.

This work includes a new unit test that evaluates almost all of the templates with test data.

The convert-cheetah script under devtools was used to partially automate the conversion, but most templates still required manual fixes on top of that.

Fixes https://pagure.io/koji/issue/4361

I squashed most of my commits, but preserved the one that renamed files to make it easier to see the transition.

Since the templates have been both substantially altered (from a linecount perspective at least) and renamed, git tends to consider that a rewrite. If you want to compare the old and new templates, then running the diff with -M05 seems to work pretty well. E.g.

  • git diff origin... -M05 --stat
  • git diff origin... -M05 'www/kojiweb/hostinfo.*'

I've run a number of comparisons between the outputs for this branch and main. The outputs are essentially the same. The major differences are:

  • whitespace variation
  • differences in html escaping (e.g. ' vs ')
  • a couple minor fixes
  • a change in default ordering when sorting by id (-id rather than id)
  • randomized greetings ;)
Edited by mikem

3 new commits added

  • alternate coverage config to check template coverage
  • move templates into subdir
  • fixes

If I run the web tests with the jinja coverage plugin enabled, we get pretty good numbers. E.g.

[mikem@localhost koji]$ PYTHONPATH=.:cli:~/Devel/coverage-jinja-plugin  coverage erase --rcfile .coveragerc3.jinja 
[mikem@localhost koji]$ time PYTHONPATH=.:plugins/hub/.:plugins/builder/.:plugins/cli/.:cli/.:www/lib:~/Devel/coverage-jinja-plugin pytest   --cov --cov-config .coveragerc3.jinja tests/test_www/
[mikem@localhost koji]$ PYTHONPATH=.:cli:~/Devel/coverage-jinja-plugin  coverage report --rcfile .coveragerc3.jinja  -i --include www/\*
Name                                             Stmts   Miss  Cover
--------------------------------------------------------------------
www/kojiweb/index.py                              1835    359    80%
www/kojiweb/templates/activesession.html.j2         15      6    60%
www/kojiweb/templates/api.html.j2                    5      0   100%
www/kojiweb/templates/archiveinfo.html.j2           39     22    44%
www/kojiweb/templates/archivelist.html.j2           26      7    73%
www/kojiweb/templates/buildinfo.html.j2             44      3    93%
www/kojiweb/templates/buildrootinfo.html.j2         18      0   100%
www/kojiweb/templates/buildrootinfo_cg.html.j2      13      0   100%
www/kojiweb/templates/buildroots.html.j2            25      0   100%
www/kojiweb/templates/builds.html.j2                54      7    87%
www/kojiweb/templates/buildsbystatus.html.j2        22      0   100%
www/kojiweb/templates/buildsbytarget.html.j2        30      6    80%
www/kojiweb/templates/buildsbyuser.html.j2          17      6    65%
www/kojiweb/templates/buildtargetedit.html.j2        9      0   100%
www/kojiweb/templates/buildtargetinfo.html.j2        8      1    88%
www/kojiweb/templates/buildtargets.html.j2          17      7    59%
www/kojiweb/templates/channelinfo.html.j2           15      0   100%
www/kojiweb/templates/clusterhealth.html.j2         14      0   100%
www/kojiweb/templates/externalrepoinfo.html.j2       6      0   100%
www/kojiweb/templates/fileinfo.html.j2              13      0   100%
www/kojiweb/templates/footer.html.j2                 4      0   100%
www/kojiweb/templates/header.html.j2                29      6    79%
www/kojiweb/templates/hostedit.html.j2              12      0   100%
www/kojiweb/templates/hostinfo.html.j2              21      4    81%
www/kojiweb/templates/hosts.html.j2                 47      6    87%
www/kojiweb/templates/index.html.j2                 40     19    52%
www/kojiweb/templates/macros.html.j2                 8      3    62%
www/kojiweb/templates/notificationedit.html.j2       9      0   100%
www/kojiweb/templates/packageinfo.html.j2           38      2    95%
www/kojiweb/templates/packages.html.j2              39     10    74%
www/kojiweb/templates/packagesbyuser.html.j2        17      6    65%
www/kojiweb/templates/recentbuilds.html.j2          14      1    93%
www/kojiweb/templates/repoinfo.html.j2              20      5    75%
www/kojiweb/templates/reports.html.j2                1      0   100%
www/kojiweb/templates/rpminfo.html.j2               51     14    73%
www/kojiweb/templates/rpmlist.html.j2               26      4    85%
www/kojiweb/templates/rpmsbyhost.html.j2            24      6    75%
www/kojiweb/templates/search.html.j2                68     25    63%
www/kojiweb/templates/tagedit.html.j2               12      0   100%
www/kojiweb/templates/taginfo.html.j2               28      6    79%
www/kojiweb/templates/taginfo_deleted.html.j2        5      0   100%
www/kojiweb/templates/tagparent.html.j2             12      0   100%
www/kojiweb/templates/tags.html.j2                  17      1    94%
www/kojiweb/templates/taskinfo.html.j2              72     25    65%
www/kojiweb/templates/tasks.html.j2                 67      8    88%
www/kojiweb/templates/tasksbyhost.html.j2           21      6    71%
www/kojiweb/templates/tasksbyuser.html.j2           17      6    65%
www/kojiweb/templates/userinfo.html.j2              29      0   100%
www/kojiweb/templates/users.html.j2                 23      6    74%
www/lib/kojiweb/__init__.py                          0      0   100%
www/lib/kojiweb/util.py                            644    126    80%
--------------------------------------------------------------------
TOTAL                                             3640    719    80%
Edited by mikem

1 new commit added

  • fix template install path

1 new commit added

  • minor output compatibility fixes

2 new commits added

  • drop custom coverage config
  • drop builder changes for now

rebased onto caeac8e7c7e6b40c0c78d915aa9eed91220d38ff

Rebased. Minor conflict with #4303. I duplicated the fix in the jinja template and verified manually

I pulled out the builder changes for now since that is the part I'm least certain about. I'll file a separate PR later for that. I think this is ready to go, maybe get it into 1.36 before it bit-rots.

There are a lot of wins here. More test coverage, vastly easier debugging (backtraces show the template code), possibility of measuring template test coverage, and generally better templating options.

There's at least one pending PR with web changes -- #4269
That one is also mine. Happy to update that one if we can get this one merged, or vice-versa.

Overall LGTM. :thumbsup:

Testing notes

This is a lot of code change, but it's not meant to be a much behavior change at all. All the code changes are confined to the web ui. The main thing is to verify that the web ui works as expected.

As I alluded to above, I tested a fairly broad list of web urls (using fakeweb, for convenience and for the ability to fake auth and timestamp). While developing this, I ran an output comparison between the old and new code for that url list. That was useful during development, but I don't know if we need to make such specific output assertions in the integration tests. It's probably enough to make sure the pages load and produce sane results.

It may be that our existing web integration tests are already close to sufficient.

And of course, there are the unit tests I added (note the CALLS list there is based on the same url list, which hits the majority of the code).

I've tried against a production env and :thumbsup:

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-basic

Metadata Update from @mfilip:
- Pull-request tagged with: testing-done

Tried to merge it and flake8/bandit fails:

# bandit
>> Issue: [B704:markupsafe_markup_xss] Potential XSS with ``markupsafe.Markup`` detected. Do not use ``Markup`` on untrusted data.
   Severity: Medium   Confidence: High
   CWE: CWE-79 (https://cwe.mitre.org/data/definitions/79.html)
   More Info: https://bandit.readthedocs.io/en/1.8.3/plugins/b704_markupsafe_markup_xss.html
   Location: www/lib/kojiweb/util.py:131:11
# flake8
./www/lib/kojiweb/util.py:89:100: E501 line too long (103 > 99 characters)
./www/lib/kojiweb/util.py:92:100: E501 line too long (109 > 99 characters)
./www/lib/kojiweb/util.py:95:100: E501 line too long (109 > 99 characters)
./www/lib/kojiweb/util.py:150:13: E126 continuation line over-indented for hanging indent
./www/lib/kojiweb/util.py:271:1: E305 expected 2 blank lines after class or function definition, found 1

# bandit

The complaint is from here:

   Location: www/lib/kojiweb/util.py:131:11
130 def SafeValue(value):
131     return Markup(value)
132

The SafeValue function is how we mark values safe. It is used a handful of places.

TBH, the B704 plugin seems overzealous and perhaps even misinformed. It appears to only allow Markup to be used on contant values or the results of specifically configured functions. However, Markup is itself intended as a way to mark values as safe. All our uses of SafeValue have been checked (during last xss cve).

We could either:

  1. disable this plugin
  2. add a do-nothing wrapper to appease the test
  3. substantially refactor the code to avoid using SafeValue
Edited by mikem

2 new commits added

  • flake8
  • fix bandit B704

I've implemented option 2 for now

Commit c057c290 fixes this pull-request

Pull-Request has been merged by tkopecek
Metadata
None

testing-basic testing-done

Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.