-
Notifications
You must be signed in to change notification settings - Fork 144
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
Export: Add custom template support #115
Export: Add custom template support #115
Conversation
6912c5f
to
d660c52
Compare
Rebased to fix merge conflict. |
@nickstenning, @slafs: What do you guys think of this? |
For me this doesn't feel right. I think the responsibility for deciding whether to use rendering with provided full_path or with the builtin path is oddly placed. Don't have much time right now. Let me think about it for a while and I'll get back to you. |
I'm not really happy with a flag like this that purports to be general but in practice only works for one export format. Part of the reason for allowing exporters to expose themselves to honcho through setuptools entrypoints is to allow exactly this kind of customisation, but in a more general way. I appreciate that this is simpler, but if it doesn't generalise it's actually adding complexity to the user interface. In addition, I think you'll find that with #119 merged, it will be much simpler to write custom exporters. |
I think I have the same feeling that it's not quite right and could be better. I think maybe the choosing of the template is happening at too low of a level. And the Export classes shouldn't be concerned with it. You should just give them a template and they should render it. After all they are exporters and not template choosers. Maybe the choice of template should happen further up the stack? This might intersect a bit with another idea I had - to allow the specified template to be an asset spec (package resource) in addition to a plain ol' absolute file path. This would allow one to If that existed, then the default honcho supervisord template could just be a default value that is a resource spec |
The problem is that some exporters use multiple templates. Have a look at the upstart exporter to see what I mean. |
Ah yes I remember the multiple template thing now. I punted on that. Not sure whether to pass a dict of templates or a directory, which seems more CLI-friendly. |
This needs to be reworked in light of the merging of #119. |
My current thought is to allow Thoughts...? |
2d1bf36
to
516df17
Compare
This is a honcho analogue of the corresponding options to `foreman export` (See http://ddollar.github.io/foreman/#EXPORTING) This lets you specify a custom Jinja template to be used when exporting to a supervisord config file. Fixes: nickstenningGH-88 See: nickstenning#88
516df17
to
afa10fd
Compare
Right now this only works for supervisor; I need to implement upstart support |
Or I suppose we could add an error message that says |
Problem with this:
will fail because it interprets the template argument as being relative to the honcho package unless it starts with a slash. Workaround: |
Or add |
Apparently, |
Do you want to rework this so the templates directory is configurable? |
@nickstenning: Just curious why you closed this? I haven't gotten around to updating this yet, but I wonder if you closed it, because you don't think it's a good idea? Are you open to another PR? |
No, it's a fine idea. Feel free to open a PR when you get a chance to update the code. |
Ok, cool, just checking before I spend the time. Thanks! |
OK, I just updated this so that
|
See #140 |
This adds the ability to use a custom template when exporting to supervisord.
Fixes: GH-88
Example: