-
Notifications
You must be signed in to change notification settings - Fork 60
Feature nginx #74
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
base: master
Are you sure you want to change the base?
Feature nginx #74
Conversation
Hey stephenrjohnson, could you take a look at the code? |
I'll get to this on the weekend, sorry about the delay. |
I'm interested in having this merged as well please @stephenrjohnson |
Hey @stephenrjohnson |
epel offers python-gunicorn.. ? why compile it? i think there should be an option to use a local package - i try to avoid compiling on production machines where i can... |
Well, unicorn isn't the same as gunicorn. In general I noticed that the rpms are most of the time outdated so I prefer gems. Also unicorn isn't available via base/epel/extras repos on CentOS7 |
bastelfreak - i just actually just realised my error there. yep.. Is it possible to have the option of using a package instead though? |
So an option to swtich between the gem or an rpm and the option to specifiy the name of the rpm? Can I assume that the rpm is available via any configured repository? would this suit your needs? |
I'm holding off merging this until you have made the changes, outlined above. |
bastelfreak, exactly 👍 in my situation id simply like to offer a rpm package name, which would avoid installing and building using gcc - etc. Soo setting the package_name it would also be important to set installpath - https://github.com/stephenrjohnson/puppetmodule/pull/74/files#diff-85eb9f3c6e80aebeebc8c583f5afd80bR5 and i noticed you arent using the puppet_passenger_port -> https://github.com/stephenrjohnson/puppetmodule/pull/74/files#diff-d52434b9535ec86f061db077733da1fbR23 there are other modules out there which manage repository management. |
Can I assume that the binary is always available in your $PATH? Your do you want to specify the complete path to the binary? I will try to add the changes later this week. are you able to write some unit/rspec tests? |
3aaa5f6
to
17bdf2d
Compare
hey @stephenrjohnson could you please take a look at this? implemented the mentioned stuff from @arenstar let me know if I should rebase it into one commit |
tested this yesterday on several nodes together with @gavinrogers and we fixed two bugs. currently it is running fine on multiple production systems |
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.
I know why this has been done but it's a backwards breaking change so we are going to have to make this a major version release.
The last think we need is some tests as this is a massive change. Have a look in spec/acceptance/ and we don't have any spec tests to test both branches of the webserver tree or even any for the unicorn class. |
anybody with a bit of experience in writing tests around that could assist me? Maybe @gavinrogers ? |
@stephenrjohnson i've started working on spec tests etc. i was curious about the acceptance tests, i noticed that one of the targets is centos7, but i haven't yet found a way to install passenger on centos7, where is the package coming from? i've got it working fine on that platform using the unicorn class, let me get the tests done |
You're writing the tests? Awesome! thanks! |
@gavinrogers If you look at the spec_helper_acceptance if osfamily == 'RedHat' |
Extremely interested in this option. The rest of our stack runs on Nginx & Unicorn/Gunicorn so this would sit within that perfectly. Obviously the progress on this change has been slow lately. Is there anything I can do to assist in merging it in? Happy to assist with puppet code, testing etc |
please please please write tests. I've no clue about that, would be awesome if you could contribute it. |
Is that the only change necessary? I'd like to see the vhost for nginx setup using jfryman/nginx. Can do that too if you'd like? |
I can take a look again and try to create a vhost config via jfryman/nginx. Do you know a bit about selinux and can maybe improve the current settings? |
I've merged all the upstream code and fixed all existing tests (other than ones not passing upstream). Whilst writing unit tests for unicorn though I noticed that none of the parameters for puppet::unicorn have default values at this point. I might have to make some more significant modifications to get it working in a sane manner. |
looks way better now, travis shows only three issues, all not based on this code. |
The issues are caused by your merges not being clean this really needs rebasing on matser. |
rebase is on its way! |
@stephenrjohnson would it be okay for you if I squash this into one commit before rebasing? |
Not the best but sure it's fine. |
bf16e24
to
9011e9c
Compare
Did a huge squash and then rebased against your master |
Why is this still breaking the tests that work before the merge. |
7fa12de
to
9f61425
Compare
this is a huge commit which adds support to install nginx on a centos7 node with unicorn backend. You can run an all-in-one node with puppet master/ca on it, or use it to configure a central ssl offloading proxy with puppet ca on it and several master backends and backups.
fb90533
to
1a3a13d
Compare
hey @stephenrjohnson, could you have a look again? Did a new rebase and fixed the two travis issues (trailing whitespace and missing variable scope). Can you tell me why travis is failling? |
Feature nginx
remove nginx::config
added missing classes
cb745f4
to
c81cfa8
Compare
add basic nginx/unicorn support. nginx vhost is currently based on a template, not on nginx::resource:vhost. tested on centos 7.