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

EZP-30170: [2.5LTS] Drop Varnish 4 + get rid of duplicated VCL in meta repo #349

Merged
merged 4 commits into from Mar 1, 2019

Conversation

andrerom
Copy link
Contributor

@andrerom andrerom commented Jan 16, 2019

Story: https://jira.ez.no/browse/EZP-30170

Additional awareness / review ping:

  • @vidarl Demo Cloud / Platform.sh impact?
  • @Plopix eZ Launchpad impact?

Todo:

  • Look into docker failure for behat

@andrerom andrerom changed the title [2.5LTS] Drop Varnish 4 + get rid of duplicated VCL in meta repo [WIP][2.5LTS] Drop Varnish 4 + get rid of duplicated VCL in meta repo Jan 16, 2019
@andrerom
Copy link
Contributor Author

@Plopix Will this change be a problem form eZ Launchpad? (aka do you need VCL file before composer install / create-project has been run)

@alongosz / @mnocon Same question for you, would it make sense to change setup_ezplatform.sh back to somehow run composer install before spinning up all containers?
And before you answer that, be aware we expect just that in solr contianer already: https://github.com/ezsystems/ezplatform/blob/master/doc/docker/Dockerfile-solr#L4 ;)

@mnocon
Copy link
Member

mnocon commented Feb 1, 2019

Sadly I don't see any other clean way, given that the file is in another repository.

@Plopix
Copy link
Contributor

Plopix commented Feb 2, 2019

@andrerom no, there is no impact for eZ Launchad as it has its own varnish.vcl which is bad actually if we want to maintain it globally in http cache bundle.

I will try to look into it, to remove this, actually, varnish is container is installed in the second round of docker-compose then it might be really simple.

@vidarl
Copy link
Member

vidarl commented Feb 6, 2019

Should be no problem for Demo Cloud / Platform.sh.

And since we run composer install before the varnish container is builded, there should be no issues getting the .vcl from vendor/

@alongosz
Copy link
Member

alongosz commented Feb 6, 2019

@alongosz / @mnocon Same question for you, would it make sense to change setup_ezplatform.sh back to somehow run composer install before spinning up all containers?
And before you answer that, be aware we expect just that in solr contianer already: https://github.com/ezsystems/ezplatform/blob/master/doc/docker/Dockerfile-solr#L4 ;)

When/where exactly is this Dockerfile used? Maybe we could split the process into app (where composer install happens) + others after? composer install was moved to be done inside app container because of a couple of reasons:

  • it sets proper permissions for environment inside docker container
  • it executes composer taking into account docker environment (this might be of no impact if Travis Trusty env is in sync with what docker container offers, but might lead to problems in the future).

I'm not in the loop when it comes to the actual issue, but if this is related to eZ Launchpad, then maybe it calls for a separate setup script? setup_ezplatform.sh is done to support Travis CI only.

@andrerom
Copy link
Contributor Author

andrerom commented Feb 6, 2019

Maybe we could split the process into app (where composer install happens) + others after?

This is what it did before (first do a build stage to pull in packages). This is also what launchpad and sales demo does, and what is required for solr contianer as well already. There are many ways to do it, either just using travis php, or run php container first, before orchestrating the whole setup in second phase.

@andrerom andrerom changed the title [WIP][2.5LTS] Drop Varnish 4 + get rid of duplicated VCL in meta repo [2.5LTS] Drop Varnish 4 + get rid of duplicated VCL in meta repo Feb 18, 2019
@andrerom
Copy link
Contributor Author

As removing the files depends on QA, I'll just deprecate it for now.

So smaller PR here ready for review @lserwatka @mnocon

@andrerom andrerom requested a review from mnocon February 18, 2019 16:06
Copy link
Member

@mnocon mnocon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok for me, one doc nitpick 😛

For Varnish to work properly with eZ, you'll need to use the following VCL as starting point:

* [eZ Platform 1.7+ with Varnish 4.x/5.x with xkey VMOD](vcl/varnish4_xkey.vcl)
Provided VCL for eZ can be found in [vendor/ezsystems/ezplatform-http-cache/docs/varnish](https://github.com/ezsystems/ezplatform-http-cache/tree/0.8/docs/varnish).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it ok to link to 0.8 here? This VCL still supports 4.0 and needs manual changes (for example: https://github.com/ezsystems/ezplatform-http-cache/blob/master/docs/varnish/vcl/varnish4.vcl#L268) and we won't be changing it in 0.8 branch I guess.

I'd rather link to a file that is tailored for 5.1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See updates here and in ezsystems/ezplatform-http-cache#77

Copy link
Member

@mnocon mnocon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks!

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs JIRA issue? :)

@andrerom andrerom changed the title [2.5LTS] Drop Varnish 4 + get rid of duplicated VCL in meta repo EZP-30170: [2.5LTS] Drop Varnish 4 + get rid of duplicated VCL in meta repo Feb 25, 2019
@andrerom
Copy link
Contributor Author

@mnocon Did you test the slightly new VCL?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants