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

Move csrgen templates into ipaclient package #534

Closed
wants to merge 2 commits into from

Conversation

tiran
Copy link
Member

@tiran tiran commented Mar 2, 2017

csrgen broke packaging of ipaclient for PyPI. All csrgen related
resources are now package data of ipaclient package. Package data is
accessed with Jinja's PackageLoader() or through pkg_resources.

https://pagure.io/freeipa/issue/6714

Signed-off-by: Christian Heimes cheimes@redhat.com

@tiran
Copy link
Member Author

tiran commented Mar 2, 2017

@LiptonB please have a look.

csrgen broke packaging of ipaclient for PyPI. All csrgen related
resources are now package data of ipaclient package. Package data is
accessed with Jinja's PackageLoader() or through pkg_resources.

https://pagure.io/freeipa/issue/6714

Signed-off-by: Christian Heimes <cheimes@redhat.com>
@@ -56,5 +63,6 @@
extras_require={
"install": ["ipaplatform"],
"otptoken_yubikey": ["yubico", "usb"]
}
},
zip_safe=False,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed? According to http://setuptools.readthedocs.io/en/latest/setuptools.html#setting-the-zip-safe-flag "And if the project uses pkg_resources for all its data file access, then C extensions and other data files shouldn’t be a problem at all." zip_safe=true seems to work when I build it locally, but I'm not sure if I'm testing it properly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, Jinja uses a pkg_resources ResourceManager to access files. So in theory, zip loading would work.

In practice the option and feature is longer relevant. Python eggs are a dying species and Python wheels no longer use zip deployment.

@LiptonB
Copy link
Contributor

LiptonB commented Mar 3, 2017

Oops, sorry about the breakage. This seems fine to me, although I hadn't really been thinking of the templates and rules as data files. They're intended to be possible to modify, more like config files. (Come to think of it, /usr/share wasn't that appropriate for them either). So, that and the fact that they're now duplicated between python2.*/site-packages and python3.*/site-packages give me pause (especially if the user might edit them), but I don't have strong feelings about it.

@tiran
Copy link
Member Author

tiran commented Mar 3, 2017

In my opinion, a user should never modify a file that managed by a package manager and not explicitly marked as a config file. Both files in /usr/share and site-packages are not config files.

How about http://jinja.pocoo.org/docs/2.9/api/#jinja2.ChoiceLoader and this idea?

loader = jinja2.ChoiceLoader(
    jinja2.FileSystemLoader(os.path.join(api.env.confdir, 'csrgen/templates')),
    jinja2.PackageLoader('ipaclient', 'csrgen/templates'),
)

This allows users to override the templates by copying them to /etc/ipa/csrgen/templates. We'd need similar code for the JSON files, too.

First try custom location, then csrgen subdir in confdir and finally
fall back to package data.

Signed-off-by: Christian Heimes <cheimes@redhat.com>
@LiptonB
Copy link
Contributor

LiptonB commented Mar 6, 2017

I think this is a much better way to make it configurable than how I had it, and the implementation looks good to me. Thanks!

@MartinBasti MartinBasti self-assigned this Mar 8, 2017
@MartinBasti MartinBasti added the ack Pull Request approved, can be merged label Mar 8, 2017
@ghost
Copy link

ghost commented Mar 8, 2017

master:

  • 80be181 Move csrgen templates into ipaclient package
  • 177f07e Chain CSR generator file loaders

@ghost ghost added the pushed Pull Request has already been pushed label Mar 8, 2017
@ghost ghost closed this Mar 8, 2017
@tiran tiran deleted the csrgen_pkgdata branch March 8, 2017 15:07
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
3 participants