Updates nginx Formula for 1.12.1 Release #51
Conversation
files/brews/nginx.rb
Outdated
|
|
||
| plist_options :manual => "nginx" | ||
|
|
||
| def plist; <<-EOS.undent |
There was a problem hiding this comment.
This plist entry isn't needed since we are already managing it via templates/dev.nginx.plist.erb
files/brews/nginx.rb
Outdated
| end | ||
|
|
||
| def install | ||
| # Changes default port to 8080 | ||
| inreplace "conf/nginx.conf" do |s| |
There was a problem hiding this comment.
This block isn't required since we are already managing it via templates/config/nginx/nginx.conf.erb and if someone wants to change the default, it can be done with hiera.
files/brews/nginx.rb
Outdated
| end | ||
|
|
||
| def post_install | ||
| (etc/"nginx/servers").mkpath |
There was a problem hiding this comment.
I don't think this is needed since Puppet should already be managing the directory structure. Can you remove it and test it out?
There was a problem hiding this comment.
Where "this" is everything inside of the post_install. Apologies for the ambiguity.
|
Thanks for the contribution! 🍰 I've got a few tweaks on this one since the the existing Homebrew formula looks to ensure everything is managed (such as directories) however we should already be managing this via Puppet.
You're not the first 😄 The idea is that Boxen would allow it's owners/maintainers to pin to specific vendored versions and ensure everyone in their organisation was running the same version. This is something that didn't used to be possible (unsure if it even is now?) to do with Homebrew so the brews are stored in the repository with the code that manages the installation. Does that make sense? |
Boxen itself handles customizations to the default port, plist generation, and directory structure, obsolescing these portions of the base Homebrew formula.
The HEAD and development branches are unnecessary, given the purpose of the custom formula (i.e. to pin a specific package version). If an end-user would like to replace their nginx installation with a HEAD or development version, this can be done easily enough directly through Homebrew.
|
Hey @jacobbednarz, thanks so much for the thorough feedback. Your explanation of the custom formulae made their purpose quite clear. Your feedback forced me, at last, to learn what the individual parts of the formulae are for. I've addressed your feedback in 77913a5, and further cleaned up some unnecessary bits in 030fac9. Comparing the custom 1.10.2 formula to the canonical Homebrew formula, I can identify some other lines that could be removed. However, I wonder whether it might be preferable to keep the Boxen formula as close to the canonical one as possible, for ease of maintenance. If every (albeit infrequent) update requires a thorough and intentional translation of the Homebrew formula, it exposes a wider surface-area for bugs. Unless the Puppet elements are being clobbered by Homebrew, why not just use Homebrew's? Let me know what you think. At any rate, as far as this update goes: in addition to the specs continuing to pass, I've dogfooded it on my local machine, as well as a clean Vagrant box, and nginx continued to behave as expected. |
| --sbin-path=#{bin}/nginx | ||
| --with-cc-opt=#{cc_opt} | ||
| --with-ld-opt=#{ld_opt} | ||
| --conf-path=#{etc}/nginx/nginx.conf |
There was a problem hiding this comment.
This breaks my version of nginx since the previous version has been built using at "--conf-path=/opt/boxen/config/nginx/nginx.conf", (from https://github.com/boxen/puppet-nginx/pull/51/files#diff-54d1ad9584e6ac1095707a81b10dc319L50).
Is this what is causing you the SIP issues?
There was a problem hiding this comment.
I just read up on the SIP directories and the problematic part will be /usr/local since Homebrew now uses that as the default. Would you be able to see where your nginx configuration file is managed and whether we can move this back under /opt/boxen?
There was a problem hiding this comment.
I've been playing with this and on newer machines, using /opt/boxen causes it not to compile whereas on older machines it works so I'll have to have a think about how we are going to combat it.
There was a problem hiding this comment.
Trying to install this package (1.7.0, same issue with 1.8.0) on OS X 10.11.6 and getting "Operation not permitted" errors when compiling. Looks related to what you mentioned above (I've tried disabling SIP but the error persists).
Have you made any progress thinking about a solution? I might be able to help out if you need it and can point me in the right direction.
There was a problem hiding this comment.
I've opened a discussion issue at boxen/boxen#214 since the configuration for Homebrew cannot be outside of the /etc directory anymore.
I'm looking into changing this across the board for all puppet modules (nginx, mysql, dnsmaq, etc) but not quite sure on the rollout plan for that just yet but I will open some PRs.
There are issues with the latest version of homebrew: * Nginx, and other services, can now only have its config in /etc * ICU4C was upgraded and is no longer compatible with El Capitan boxen/puppet-nginx#50 boxen/puppet-nginx#51
There are issues with the latest version of homebrew: * Nginx, and other services, can now only have its config in /etc * ICU4C was upgraded and is no longer compatible with El Capitan boxen/puppet-nginx#50 boxen/puppet-nginx#51
In boxen/puppet-nginx#51 we were receiving "Operation not permitted" errors when trying to install nginx configuration files to `/opt/boxen/config` due to a restriction introduced in Homebrew 1.3.0[1]. To address the installation issue, we need to move the configuration files to `/etc` however to maintain a "what does Boxen manage" list we're going to chuck it in `boxen` sub directory. The end result should be: ``` /etc/boxen/:service ``` [1]: boxen/boxen#214 (comment)
In boxen/puppet-nginx#51 we were receiving "Operation not permitted" errors when trying to install nginx configuration files to `/opt/boxen/config` due to a restriction introduced in Homebrew 1.3.0[1]. To address the installation issue, we need to move the configuration files to `/etc` however to maintain a "what does Boxen manage" list we're going to chuck it in `boxen` sub directory. The end result should be: ``` /etc/boxen/:service ``` [1]: boxen/boxen#214 (comment)
In boxen/puppet-nginx#51 we were receiving "Operation not permitted" errors when trying to install nginx configuration files to `/opt/boxen/config` due to a restriction introduced in Homebrew 1.3.0[1]. To address the installation issue, we need to move the configuration files to `%{::homebrew_root}/etc` however to maintain a "what does Boxen manage" list we're going to chuck it in `boxen` sub directory. The end result should be: ``` /usr/local/etc/boxen/:service ``` [1]: boxen/boxen#214 (comment)
This PR proposes to update the nginx formula so the module installs nginx 1.12.1 instead of 1.10.2. This was motivated by issues on El Capitan machines with SIP enabled erroring out on installation.
If I have any reluctance about this, it's because I don't understand the how and why of the custom formula. If it's about "pinning" the specific version, then this should be okay. If there are more subtle, Puppet- or Boxen-specific tweaks that need to be made, then this may not be okay. (If it's about neither of these, why not just use the Homebrew provider? 🤔)
I'd be deeply grateful for any insight into this. The custom formulae in different modules have long perplexed me!