-
-
Notifications
You must be signed in to change notification settings - Fork 881
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
Add fastcgi index #1062
Add fastcgi index #1062
Conversation
@elmobp can you add tests? |
Sure! I think the location bits needs some work though So we could do something along the lines of I will look at doing this in another PR though |
@elmobp That sounds very reasonable. Will you be able to add spec tests for this particular PR though? |
hey @wyardley sorry been tied up on a project I will get these done Wednesday |
@wyardley hows that look? |
@elmobp Sorry, didn't mean to rush you! Test looks good, voxpupuli usually asks that the commits be squashed back to a single one (rebase / force-push the branch). I'll make one other request, sorry that I didn't notice it before. |
manifests/resource/server.pp
Outdated
@@ -254,6 +255,7 @@ | |||
Optional[String] $proxy_buffering = undef, | |||
Array $resolver = [], | |||
Optional[String] $fastcgi = undef, | |||
$fastcgi_index = undef, |
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.
Can you prefix this with Optional[String] to match the rest?
Done! |
@elmobp Looks good to me, sorry there were so many hoops to jump through on this one. Merging. |
@@ -159,7 +161,7 @@ | |||
is_expected.to contain_apt__source('nginx').with( | |||
'location' => "https://nginx.org/packages/#{operatingsystem.downcase}", | |||
'repos' => 'nginx', | |||
'key' => '573BFD6B3D8FBC641079A6ABABF5BD827BD9BF62' | |||
'key' => { 'id' => '573BFD6B3D8FBC641079A6ABABF5BD827BD9BF62' } |
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 think this was from another commit? Next time, rebase rather than merge master.
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.
Oops!
@@ -37,21 +37,21 @@ | |||
apt::source { 'nginx': | |||
location => "https://nginx.org/packages/${distro}", | |||
repos => 'nginx', | |||
key => '573BFD6B3D8FBC641079A6ABABF5BD827BD9BF62', | |||
key => {'id' => '573BFD6B3D8FBC641079A6ABABF5BD827BD9BF62'}, |
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.
Here too... @bastelfreak is this Ok, since this is one that was already merged, just kind of confusing commit message.... Or should I revert?
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.
Want a fresh PR? to fix them
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.
@elmobp yeah, let's just do that.
Add fastcgi index
Add fastcgi index
Add the fastcgi index to the server resource