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

Update Ubuntu 16.04 nginx install doc with a better nginx config #6836

Merged
merged 3 commits into from Jun 22, 2017

Conversation

Projects
None yet
6 participants
@ndum
Contributor

ndum commented Jun 14, 2017

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

@CLAassistant

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant Jun 14, 2017

CLA assistant check
All committers have signed the CLA.

CLAassistant commented Jun 14, 2017

CLA assistant check
All committers have signed the CLA.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@f0o

This comment has been minimized.

Show comment
Hide comment
@f0o

f0o Jun 15, 2017

Member

Although the API rule is not inherently wrong, we chose namer location for a reason. Our API is versioned and in due time we need to support /v0 /v1 etc. This is why we had a rewrite with named location instead of matching the actual directoryand try_files

Member

f0o commented Jun 15, 2017

Although the API rule is not inherently wrong, we chose namer location for a reason. Our API is versioned and in due time we need to support /v0 /v1 etc. This is why we had a rewrite with named location instead of matching the actual directoryand try_files

@f0o

API rules break versioning support

@ndum

This comment has been minimized.

Show comment
Hide comment
@ndum

ndum Jun 16, 2017

Contributor

Hi,

Please correct me, but is v0 actually not hardcoded also?

rewrite api/v0(.*)$ /api_v0.php/$1 last;

if u need v0 / v1 just add a second location? (as example)

location /api/v0 {
 try_files $uri $uri/ /api_v0.php?$query_string;
}

location /api/v1 {
 try_files $uri $uri/ /api_v1.php?$query_string;
}

This named location does not works in all situation, that is why i created this MR. In addition it's actually match both rules every time and this is bad for the performance.

Thanks for your cooperation.

Contributor

ndum commented Jun 16, 2017

Hi,

Please correct me, but is v0 actually not hardcoded also?

rewrite api/v0(.*)$ /api_v0.php/$1 last;

if u need v0 / v1 just add a second location? (as example)

location /api/v0 {
 try_files $uri $uri/ /api_v0.php?$query_string;
}

location /api/v1 {
 try_files $uri $uri/ /api_v1.php?$query_string;
}

This named location does not works in all situation, that is why i created this MR. In addition it's actually match both rules every time and this is bad for the performance.

Thanks for your cooperation.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jun 16, 2017

Member

Does your change support /api/v0 though, it looks like it will pass v0 as a parameter to api_v0.php. Maybe just be explicit with the v0 in the rule?

Member

laf commented Jun 16, 2017

Does your change support /api/v0 though, it looks like it will pass v0 as a parameter to api_v0.php. Maybe just be explicit with the v0 in the rule?

@ndum

This comment has been minimized.

Show comment
Hide comment
@ndum

ndum Jun 16, 2017

Contributor

Does your change support /api/v0 though, it looks like it will pass v0 as a parameter to api_v0.php. Maybe just be explicit with the v0 in the rule?

Yes, if you send a request (example: xxx.xxx.org/api/v0/devices) the "devices" will be passed as a parameter.

$query_string is just the last input parameter, so it will send "devices" to api_v0.php.
example: api_v0.php?devices
`
Like i said before, if you want another API location (v1 / v2 or what ever), just add this as a new location.

Contributor

ndum commented Jun 16, 2017

Does your change support /api/v0 though, it looks like it will pass v0 as a parameter to api_v0.php. Maybe just be explicit with the v0 in the rule?

Yes, if you send a request (example: xxx.xxx.org/api/v0/devices) the "devices" will be passed as a parameter.

$query_string is just the last input parameter, so it will send "devices" to api_v0.php.
example: api_v0.php?devices
`
Like i said before, if you want another API location (v1 / v2 or what ever), just add this as a new location.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jun 17, 2017

Member

Imho I think you should change it to location /api/v0 { now so that it's clear.

Member

laf commented Jun 17, 2017

Imho I think you should change it to location /api/v0 { now so that it's clear.

@ndum

This comment has been minimized.

Show comment
Hide comment
@ndum

ndum Jun 17, 2017

Contributor

I've changed it now to /api/v0

Contributor

ndum commented Jun 17, 2017

I've changed it now to /api/v0

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jun 17, 2017

Member

Brilliant thanks.

Will fix when we work out what Travis changed :/

Member

laf commented Jun 17, 2017

Brilliant thanks.

Will fix when we work out what Travis changed :/

@ndum

This comment has been minimized.

Show comment
Hide comment
@ndum

ndum Jun 17, 2017

Contributor

Perfect! Thanks ;)

Contributor

ndum commented Jun 17, 2017

Perfect! Thanks ;)

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jun 19, 2017

Member

This breaks access to /api-access/ for me.

Member

laf commented Jun 19, 2017

This breaks access to /api-access/ for me.

@laf laf added the Blocker 🚫 label Jun 19, 2017

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jun 22, 2017

Member

@ndum Please see my last comment.

Member

laf commented Jun 22, 2017

@ndum Please see my last comment.

@ndum

This comment has been minimized.

Show comment
Hide comment
@ndum

ndum Jun 22, 2017

Contributor

Hi @laf,
I'm looking on it,
On 2-3 installs at my environment it works fine. Also the /api-access/
I will check this out.

Edit: You have used my latest commit or?bea5cf7

Contributor

ndum commented Jun 22, 2017

Hi @laf,
I'm looking on it,
On 2-3 installs at my environment it works fine. Also the /api-access/
I will check this out.

Edit: You have used my latest commit or?bea5cf7

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jun 22, 2017

Member

Sorry no I didn't see the change.

Works fine. Can you please update the CentOS install docs as well, should be the same changes but we need to ensure uniformed info.

Member

laf commented Jun 22, 2017

Sorry no I didn't see the change.

Works fine. Can you please update the CentOS install docs as well, should be the same changes but we need to ensure uniformed info.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jun 22, 2017

Member

Oh and please rebase to fix the travis issue.

Member

laf commented Jun 22, 2017

Oh and please rebase to fix the travis issue.

@ndum

This comment has been minimized.

Show comment
Hide comment
@ndum

ndum Jun 22, 2017

Contributor

I have updated CentOS documentation now also. (see 198ccd3)

Thanks @laf ;)

Contributor

ndum commented Jun 22, 2017

I have updated CentOS documentation now also. (see 198ccd3)

Thanks @laf ;)

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Jun 22, 2017

The inspection completed: No new issues

The inspection completed: No new issues

@laf laf merged commit fb548a9 into librenms:master Jun 22, 2017

2 of 3 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jun 22, 2017

Member

Thanks very much :)

Member

laf commented Jun 22, 2017

Thanks very much :)

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 17, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

lock bot commented May 17, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 17, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.