-
Notifications
You must be signed in to change notification settings - Fork 82
Implement custom blocks to be inserted in nginx config. #142
Conversation
@@ -103,9 +103,11 @@ | |||
end | |||
end | |||
|
|||
custom_configuration = app_info["nginx_custom"].reject{ |k,v| v.nil? || v.empty? } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused block argument - k
. If it's necessary, use _
or _k
as an argument name to indicate that it won't be used.
Line is too long. [86/80]
Space missing after comma.
Space missing to the left of {.
variables( | ||
name: app, | ||
domain_names: app_info["domain_names"], | ||
enable_ssl: File.exists?("#{applications_root}/#{app}/shared/config/certificate.crt"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line is too long. [94/80]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixing this involves a lot more then just wrapping the line.
Proper fix would be to either introduce some helper modules and call methods such as certificate_exists_for?(app)
.
The next best thing, and more chef-ish, is to make these blocks into LWRPs. That allows for a lot of cleaning up.
The next best thing is to extract all these into variables, but that really does not make the code more readable.
Alll this is is a much larger, and different task than what I'm trying to achieve in this PR,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's fine. I think I'm going to ignore the Hound line length setting and set it to 150 or something like that.
Also: agree with you on refactoring this into nice cheffy libraries and LWRPs. But let's first see what the best approach is right now.
aedf3d0
to
4da8b69
Compare
One suggestion from my part would be to make the Do you have a use case for these generic |
One thing I've used them for in the past is to allow custom server {} blocks. Those server-blocks can then contain:
The fist two could best be actual features in the configuration json, something like The alternative-backends is mostly a hack to work around a (temporary) case which, arguably, should be dealt with in the app itself. But when you can add arbitrary server blocks, you can solve it this way too. |
Thanks this makes sense. Especially for the redirect stuff. I would agree with also providing "redirect_domains" but I've also noted that are so much edge cases you might want to cover that sometimes just providing the nginx snippet yourself is much better. Let's experiment with this approach. I'll test it and merge it in! Allright? |
You don't want to wait untill I've renamed them to before_server and after_server? Because I can do that, but probably not before thursday. |
@berkes If you like I can add a commit to your branch by pulling it from your remote. Would that be allright? |
yea, sure! |
Is there a way to merge this PRs on GitHub? I would like to retain this PR to do the changes :) |
@michiels Not sure what you mean? You can push the "Merge Pull Request" button? |
I want to collaborate with @berkes on his branch by adding a commit myself. But the only way to do that is create a new branch in this repository and make a new PR of that, losing this discussion and PR in the process. |
What do you guys think of some documentation? I'm a bit afraid with cluttering the README with all kinds of "advanced" features. Maybe just use the Wiki pages to explain features like these? |
Documentation. Below should go into nginx_customThe
The template is the best reference where these will go. Below is a simplified
Typical uses of the custom places:
ExampleGiven a file with redirects in
You can now include that file in the vhost with (note the trailing
Resulting in a vhost:
You now have a file where you can maintain custom redirects for TroubleshootingThe contents of the custom_conf will be placed in there literally. So, |
Signed-off-by: Jeroen van Baarsen <[email protected]>
This fixes intercity#150.
@@ -1,7 +1,7 @@ | |||
name "backups" | |||
maintainer "Firmhouse" | |||
maintainer_email "[email protected]" | |||
license "Apache 2.0" | |||
license "MIT" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put one space between the method name and the first argument.
Fixing the mergecomflicts within this PR will introduce a lot of needless commits. I'll rebase this on master and on @michiels branch and then recreate a new PR. |
@berkes Ok thanks! |
This is a go at #97