Skip to content
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

[WIP] Fixing Nginx support #60

Closed
wants to merge 8 commits into from
Closed

[WIP] Fixing Nginx support #60

wants to merge 8 commits into from

Conversation

hpatoio
Copy link
Contributor

@hpatoio hpatoio commented May 7, 2014

  • I've fixed the tests as you suggested by adding a resetProxyDaemon
  • I honestly don't like the way cache is purged for Varnish. Why don't we use varnishadm ?

Tests are still red and I need to modify how purge generate the purging URL. A fix is coming

hpatoio and others added 7 commits April 21, 2014 11:39
First draft for doc and the basic conf

Nginx class now extends AbstractCacheProxy #34 - Added purging test for different location setup

Fixing Scrutinizer issue

Update NginxTestCase.php

Refresh method and test

Removed empty line

Few more CS fix

Added nginx as keyword

Added nginx to Travis

Added HTTPS

Typo

Removed empty line

added nginx installation script on travis before_script

Nginx custom build script

Few fix for nginx installation process

Correct path

Correct path

Merging classes Sperate/Same Location

Removed useless config directives

All NGINX files are now set to /tmp

Set more NGINX conf to /tmp/

Install custom NGINX the other way

Update install-nginx.sh

Fixing permissions

New method addRiskyTest required for phpunit >= 4

Installation is needed

Add shebang

Remove NginxSeparateLocation

Also support same location
- Expanded purge & refresh tests and new expired test
- Remove cache TTL in proxy_cache_valid (as suggested by @ddeboer)
- Custom log for better debug
- Improve installing nginx for testing
@ddeboer
Copy link
Member

ddeboer commented May 7, 2014

@hpatoio Do you mean the way the Varnish cache is emptied? What don't you like about it?

@hpatoio
Copy link
Contributor Author

hpatoio commented May 7, 2014

Well. It just seems a trick to me. If stop/start the daemon has also other effects strange behavior might happen. The user think that he is only clearing the cache but he is actually restarting the daemon.

@ddeboer
Copy link
Member

ddeboer commented May 7, 2014

As the clearCache() method is only available on the test classes, and not on any production code, I don't think that'll be a problem.

@dbu
Copy link
Contributor

dbu commented May 8, 2014

i think its fine for the test that we restart varnish to be sure everything is gone. everything else would rely on the vcl being correctly set up with banning. unless we would implement the varnishadm communication port stuff in FriendsOfSymfony/FOSHttpCacheBundle#14 (and then all tests would depend on that bit working correctly...)

@dbu dbu added the nginx label May 8, 2014
@hpatoio
Copy link
Contributor Author

hpatoio commented May 10, 2014

One questions for @dbu : what did you want to test with testPurgeSeparateLocationPath and testPurgeSameLocationPath in NginxTest.php ?

@dbu
Copy link
Contributor

dbu commented May 11, 2014

@hpatoio one is sending the hostname along, the other only a path, meaning the default host needs to be added in the right place.

@hpatoio
Copy link
Contributor Author

hpatoio commented May 13, 2014

Well, I don't understand why I should specify the hostname when I purge an URL. Purge requests are sent to the proxies using IPs not application's hostname so why specify it ? Just to remove it ?

@dbu
Copy link
Contributor

dbu commented May 14, 2014

lests assume i have site-a.com and site-b.com and both have /page. now i want to purge site-a.com/page but not site-b.com/page.

@ddeboer ddeboer added this to the 1.0 milestone May 16, 2014
@dbu
Copy link
Contributor

dbu commented May 17, 2014

merged manually to #49

@dbu dbu closed this May 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants