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

Allow template to be a whole directory #57

Merged
merged 3 commits into from
Dec 1, 2016
Merged

Conversation

breml
Copy link
Contributor

@breml breml commented Sep 26, 2016

Use-case: process complete conf.d directories without the need to list every file separately.

Copy link
Owner

@jwilder jwilder left a comment

Choose a reason for hiding this comment

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

Can you update the README with details on how this work? If a dir is passed in, does it just create the new generated file in the dest dir using the name of file in the src dir as the template?

@breml
Copy link
Contributor Author

breml commented Sep 27, 2016

@jwilder just updated the README.

@breml
Copy link
Contributor Author

breml commented Oct 10, 2016

@jwilder is there still something missing for this PR to be merged?

@jmahowald
Copy link

jmahowald commented Oct 30, 2016

doh, should have checked before I coded .
@breml I also created #64.
Maybe we take my tests and use your implementation and readme?

@breml
Copy link
Contributor Author

breml commented Oct 31, 2016

@jwilder could you please join and let us know what do you think about this PR and #64 and how you want us to precede.

@kramarz
Copy link

kramarz commented Oct 31, 2016

I am also loking for this feature simply because i have sites_enabled in my nginx, and everyone of them i need to transform into a template. Glad there is PR already.

@breml
Copy link
Contributor Author

breml commented Nov 16, 2016

@jwilder would love to get some feedback.

@breml
Copy link
Contributor Author

breml commented Nov 30, 2016

@jwilder I have updated the README, as you asked me to do. how do you want me to proceed?

Copy link
Owner

@jwilder jwilder left a comment

Choose a reason for hiding this comment

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

@breml Sorry for the long delay. A few small nits, but otherwise looks good.

log.Fatalf("unable to stat %s, error: %s", template, err)
}
if fi.IsDir() {
if dest != "" {
Copy link
Owner

Choose a reason for hiding this comment

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

Could you move this whole block L241-L262 to a separate func like generateDir?


for _, file := range files {
if dest == "" {
generateFile(template+string(os.PathSeparator)+file.Name(), "")
Copy link
Owner

Choose a reason for hiding this comment

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

You can use filepath.Join(template, file.Name) here

if dest == "" {
generateFile(template+string(os.PathSeparator)+file.Name(), "")
} else {
generateFile(template+string(os.PathSeparator)+file.Name(), dest+string(os.PathSeparator)+file.Name())
Copy link
Owner

Choose a reason for hiding this comment

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

Ditto: file path.Join(...) for these.

Use filepath.Join
Implement review feedback from jwilder
@breml
Copy link
Contributor Author

breml commented Dec 1, 2016

@jwilder just implemented your suggestions.

Copy link
Owner

@jwilder jwilder left a comment

Choose a reason for hiding this comment

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

Looks great!

@jwilder jwilder merged commit e791517 into jwilder:master Dec 1, 2016
@jwilder
Copy link
Owner

jwilder commented Dec 1, 2016

Thanks @breml!

@jwilder jwilder mentioned this pull request Dec 1, 2016
@breml breml deleted the templateDirs branch December 2, 2016 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants