Pongo2 support #18
Pongo2 support #18
Conversation
tcpj
commented
Apr 25, 2018
tcpj
commented
- Support for pongo2 template system (jinja2-like)
- Slightly refactored for better readability
There was a problem hiding this comment.
- Generally ok
- Templar? Is it a real world? :D
- I would prefer to change the directory structure to create a file just with an interface and separate files with implementation in a subfolder(s). Although I am not really sure what is the standard way, but this is messy.
pongo.go
Outdated
| "os" | ||
| "strings" | ||
|
|
||
| "github.com/flosch/pongo2" |
goenvtemplator.go
Outdated
| flag.BoolVar(&doExec, "exec", false, "Activates exec by command. First non-flag arguments is the command, the rest are it's arguments.") | ||
| flag.BoolVar(&printVersion, "version", false, "Prints version.") | ||
| flag.Var(&envFileList, "env-file", "Additional file with environment variables. Can be passed multiple times.") | ||
| flag.StringVar(&delimLeft, "delim-left", "", "Override default left delimiter {{.") |
There was a problem hiding this comment.
You should at least mention, that delims are used only with default engine. And maybe we should think a way how to make this more general.
There was a problem hiding this comment.
Maybe a custom help for every engine? Every engine could have its own options. I'll think about it.
goenvtemplator.go
Outdated
| templar = &PongoTemplar{ | ||
| Source: source, | ||
| } | ||
| default: |
There was a problem hiding this comment.
What about failing in default and have a case "text/template"?
There was a problem hiding this comment.
That breaks backwards compatibility. You would have to change every Dockerfile with goenvtemplator entrypoint... for example.
There was a problem hiding this comment.
No, I mean
+ flag.StringVar(&engine, "engine", "text/template", "Override default text/template [support: pongo2]")
and to have a case "text/template" and default if an user passes --engine=foo-bar to fail and not to use default text/template engine.
There was a problem hiding this comment.
Truly looks better and more deterministic. Will fix.
There was a problem hiding this comment.
+1 there should be sanity check on the engine type or at least warning about using the default go template engine
goenvtemplator.go
Outdated
| if err := generateTemplates(tmpls, debugTemplates, delimLeft, delimRight); err != nil { | ||
| log.Fatal(err) | ||
| if err := generateTemplates(tmpls, delimLeft, delimRight, engine); err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
Can you please explain the reason of the change?
There was a problem hiding this comment.
I think it is better to panic than proceed to execute a command with partially generated templates.
There was a problem hiding this comment.
https://golang.org/pkg/log/#Fatal
Fatal calls os.Exit(1) so it will not proceed.
pongo.go
Outdated
| @@ -25,6 +26,8 @@ func (templar *PongoTemplar) generateTemplate() (string, error) { | |||
| context[key] = value | |||
| } | |||
|
|
|||
| Debug("Using context %v", context) | |||
There was a problem hiding this comment.
I was not a fan the co called "debug" feature, and I am still not. There might be a sensitive information you will be printing...
| t.destination = strings.TrimSpace(parts[1]) | ||
| } else { | ||
| parts := strings.SplitN(value, ":", 2) | ||
| if len(parts) < 2 { |
There was a problem hiding this comment.
I actually don't like this change...
If you pass foo:bar:tralala than everything is working even thought the tralala is omitted and user gets no invoice about this.
There was a problem hiding this comment.
Sorry, I'm taking back this comment. I've misunderstood how the SplitN function works.
goenvtemplator.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func readSource(templatePath string) (string, error) { |
There was a problem hiding this comment.
I woud prefer naming this readFile since you already have writeFile
goenvtemplator.go
Outdated
| templar = &PongoTemplar{ | ||
| Source: source, | ||
| } | ||
| default: |
There was a problem hiding this comment.
+1 there should be sanity check on the engine type or at least warning about using the default go template engine
goenvtemplator.go
Outdated
| func writeFile(destinationPath string, data string) error { | ||
| if !filepath.IsAbs(destinationPath) { | ||
| log.Fatalf("Destination path '%s' is not absolute!", destinationPath) | ||
| return errors.New("absolute path error") |
There was a problem hiding this comment.
This line is dead code, I would prefere to remove the Fatal logging line.
|
LGTM, can you please squash, update changelog and write docs? |
|
sure i can |
