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

Updates MFTF configuration with notes about some common issues #268

Closed
wants to merge 3 commits into from

Conversation

Lunaetic
Copy link
Contributor

@Lunaetic Lunaetic commented Dec 4, 2020

Updates mftf.md with several notes regarding getting it running in a default magento2 environment type.

@Lunaetic
Copy link
Contributor Author

Lunaetic commented Dec 4, 2020

Hopefully my formatting here is acceptable - I wasn't sure where the information should go - if anyone has a better suggestion, just holler at me and I'll adjust.

Copy link
Collaborator

@davidalger davidalger left a comment

Choose a reason for hiding this comment

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

Lots of good info here, thank you! Posed a number of questions inline with the changes. The driving force behind them is: Let's get these necessary changes into the base templates for Selenium support, unless there is a good reason not to, rather than making the setup require more manual effort to function.

extra_hosts:
- ${TRAEFIK_SUBDOMAIN:-app}.${TRAEFIK_DOMAIN}:${TRAEFIK_ADDRESS:-0.0.0.0}
environment:
- START_XVFB=false
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Lunaetic @lbajsarowicz Any reason this change couldn't/shouldn't be simply made in the Warden default environment spec for MFTF? Doesn't make sense to me to include a broken Selenium setup (even if it's not really our fault) and then have a doc saying how to override it with one that works.


```
client_max_body_size 100M;
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be embedded in the /etc/nginx/nginx.conf within the Nginx image. No reason users should need to set a custom Nginx setting here. Looking at the role I've used for Nginx in production, the default value I've used there has been 200m, so let's go with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clarification - when you say "embedded in the nginx.conf within the Nginx image"; do you mean "added to the docker.io image"?

Copy link
Contributor Author

@Lunaetic Lunaetic Dec 15, 2020

Choose a reason for hiding this comment

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

I've created the following images using wardenenv/nginx:1.16 and wardenenv/nginx:1.17 as bases that include this change in the nginx.conf files - I've tested them, however I'm not sure what the process is to update the containers in warden. Do they get pulled into the wardenenv/ repo, or is updating the base.nginx.yml file to point at them acceptable?

https://hub.docker.com/repository/docker/lunaetic/nginx

version: "3.5"
services:
selenium-chrome:
shm_size: '1gb'
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Lunaetic @lbajsarowicz Is this shm_size fairly static or can it fluctuate? I.e. if we change the MFTM support to use chrome vs the spoke/hub (as it is currently) the shm_size should just be set in the defaults. If it's a fairly static need for this to be 1gb, then it can simply be used, otherwise it could be made a variable. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1gb was an arbitrary size I chose while working on it; variable works, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a variable, set to 128mb, which still allows the tests to run from my testing.

@Lunaetic
Copy link
Contributor Author

@davidalger Gotcha - I'll update this PR with the suggested changes this week

@davidalger
Copy link
Collaborator

Closing in favor of #349

@davidalger davidalger closed this Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants