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

feat: add an additional Caddyfile config template to the existing template #50

Merged
merged 14 commits into from
Apr 12, 2021

Conversation

couetilc
Copy link
Contributor

@couetilc couetilc commented Apr 9, 2021

This could be a potential fix for #2 , not sure if you all would prefer to add to the existing template, or replace it altogether. There could be separate env var for each option. What do you think of these changes? (explanation in README changes)

@couetilc
Copy link
Contributor Author

couetilc commented Apr 9, 2021

I could also add a CADDY_FILE envvar that will replace the default Caddyfile.tmpl in a similar manner to how CADDY_SNIPPET is appended to the default Caddyfile. So there would be two options for custom templates: CADDY_FILE which will replace the default template, and CADDY_SNIPPET which will append to the existing template. Unfortunately, I do not think it's easy to support adding multiple snippets.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

Two questions:

  1. Is it possible to test this easily?
  2. Can you please rebase you PR on master?

@couetilc
Copy link
Contributor Author

couetilc commented Apr 10, 2021

I'll think of a way to test, I'll update the .travis.yml file

@couetilc
Copy link
Contributor Author

@sobolevn I rebased with master and added tests to the CI, any feedback is welcome.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Now, please you review my sh style fixes.
If that's ok, I would merge it right away! 👍

@sobolevn
Copy link
Member

Looks like I broke something 😢

@couetilc
Copy link
Contributor Author

Looks good I think we can merge, thank you for the feedback @sobolevn . I will update docs today for #52 demonstrating how to set up a file_server with the new options, then rebase, and add a feature to include snippets using container labels. We should be good to release after that.

snippet definitions can be easily inserted for use in template
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks! Great work!

@sobolevn sobolevn merged commit 4b453a2 into wemake-services:master Apr 12, 2021
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.

2 participants