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

Templates should just be part of the compiled binary, ideally. #20

Closed
morrowc opened this issue Jan 3, 2022 · 6 comments
Closed

Templates should just be part of the compiled binary, ideally. #20

morrowc opened this issue Jan 3, 2022 · 6 comments
Labels
enhancement New feature or request

Comments

@morrowc
Copy link
Contributor

morrowc commented Jan 3, 2022

Having the templates read at run-time means that the current working directory when launching the binary
must have a `./templates/...' with the appropriate content included. If this isn't the case you get:

$ /usr/local/bin/dmarc-report-converter  -config /etc/dmarc-reporting/config.yaml
panic: open ./templates/html_static.gotmpl: no such file or directory

goroutine 1 [running]:
main.loadTemplate(0x8c1c680, 0x1e, 0xc)
	/usr/local/bin/config.go:63 +0xf6
main.loadConfig(0xbff2ec2a, 0x43, 0x2, 0x3, 0x0)
	/usr/local/bin/config.go:90 +0x11c
main.main()
	/usr/local/bin/main.go:30 +0x159
$ 

how about discussion-over-code-review? :)

morrowc added a commit to morrowc/dmarc-report-converter that referenced this issue Jan 3, 2022
…sts.

Extract the template text/html into constants.
Reference the constants instead of the file read content.

Next, remove 'loadTemplate' since it will not be used anylonger.
morrowc added a commit to morrowc/dmarc-report-converter that referenced this issue Jan 3, 2022
@tierpod
Copy link
Owner

tierpod commented Feb 1, 2022

@morrowc Thank you for your idea and PR! Sorry for late answer.

I see that searching templates in current working directory is a bad idea.

Your way has many advantages for end user:

  • it's much more reliable
  • it's easier to install, you don't have to prepare templates directory

And one disadvantage:

  • It's harder to change templates, you have to recompile binary for that (although I haven't changed templates for a long time, I think it is convenient feature to use for development at least).

Maybe we can keep your changes and add another output format for using external template file? What do you think? Something like this:

output:
  format: "external_template"
  external_template: "/path/to/file.gotmpl"

@tierpod tierpod added the enhancement New feature or request label Feb 1, 2022
@morrowc
Copy link
Contributor Author

morrowc commented Feb 1, 2022

howdy! no worries on the delays...

I think there are 2 things going on:

  1. an assumption about directory for deployment/runtime
  2. a normal form/style change to include the template content at compile time instead of rationalizing how to do 1 better.

You mention: "easier for development", which seems specious to me, since unless you included a ton of random/extra data in the template data struct, you'd always have to reompile to get data included and output that in the template(s) output?
(unless you mean just changing the free-text... which seems like a fairly light load of trade-off to me).

@tierpod
Copy link
Owner

tierpod commented Feb 2, 2022

unless you mean just changing the free-text

Yes, I meant this. For me, data struct is almost static and isn't changed a lot. But html or txt templates can be changed a lot during development process or even in production usage. I don't know, maybe someone already uses self-written custom templates. That's why I would like to keep ability to use external template via another output format.

Anyway, your PR is good, I'm going to merge it soon.

@morrowc
Copy link
Contributor Author

morrowc commented Feb 2, 2022

ah! ok, I see your usecase now. If you merge this and send me an issue I'll put in a pr that we can discuss?

tierpod added a commit that referenced this issue Feb 3, 2022
This should address #20, move the template content into the compiled binary.
@tierpod
Copy link
Owner

tierpod commented Feb 3, 2022

I merged your PR and added external_template output format. Thanks!

@tierpod tierpod closed this as completed Feb 3, 2022
@tierpod
Copy link
Owner

tierpod commented Feb 3, 2022

New version v0.6 was released

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants