Test ecommerce via the container built in configuration. #1026
Conversation
feanil
force-pushed
the
feanil/ecommerce_docker_testing
branch
4 times, most recently
from
e926904
to
f7b53d7
Compare
feanil
changed the title
| @@ -0,0 +1,16 @@ | |||
| # This is only meant to be used for testing on travis-ci.org | |||
There was a problem hiding this comment.
I'd like to keep the structure of these files consistent across our repos. In programs, we have a docker-compose-travis.yml at the root of the repo. Here, the file lives in a .travis directory. Either is fine with me as long as we're consistent. Looks like a .travis directory was introduced to programs as well, so it could just be a matter of moving the docker-compose-travis.yml file on that end.
Related: these files should be added to https://github.com/edx/cookiecutter-django-ida/.
There was a problem hiding this comment.
Agreed, @jibsheet thoughts on the location of this file. I felt that since the other travis files were hidden, this should be as well.
There was a problem hiding this comment.
I think moving programs into .travis/ is a good idea. Docker compose was a late breaking change so I didn't think about location as much for that file (other than trying to make it clear that it was travis specific in the name).
There was a problem hiding this comment.
Per this discussion: edx/programs#179
| # run and expose a port for ecommerce. This means that we need to run some | ||
| # command that keeps the ecommerce container alive while we run tests on it. | ||
| # We have not yet standardized on an init replacement which could be used instead. | ||
| command: tail -f /dev/null |
There was a problem hiding this comment.
Thoughts on using https://github.com/krallin/tini for this?
There was a problem hiding this comment.
There are several competing "tiny initd" projects. I like tini, but other folks have other opinions. This is why i had settled on the hack for this project. If Feanil wants to tackle switching, great, but that's the reason I hadn't done tini last time.
There was a problem hiding this comment.
Seems fine for when we actually care what process is running in the container. Thinking about it a bit more, it seems like even if we did have an init replacement, we would still run the tail here to keep the container running.
| . /edx/app/ecommerce/nodeenvs/ecommerce/bin/activate | ||
|
|
||
| apt update | ||
| apt install -y xvfb firefox gettext |
There was a problem hiding this comment.
We should install this as part of image creation. gettext is definitely needed for developing translations. firefox is needed for local runs of Selenium.
There was a problem hiding this comment.
The image is created from the default settings which are the production settings(no dev requirements either). I don't think it makes sense to install these in production which is why I put them here. Are you suggesting making this a part of the dockerfile definiton? @jibsheet @e0d thoughts on this. I think that since the Dockerfiles are meant for development, this is fine but it will mean one more thing to deal with if we want to separate out test/dev settings from production settings. One option may be to add to the ansible-overrides.yml for ecommerce to add these packages.
There was a problem hiding this comment.
Correct, I'm advocating for putting these in Dockerfile.
There was a problem hiding this comment.
I admit I do not have strong feelings (obviously I installed gettext into the programs Dockerfile) although much of me wants to avoid having dev tools installed on production images.
If we can do it with ansible-overrides.yml I would be +1 on that, and would swap programs around.
It'll lead to some sort of bug later where we don't install something in prod, but I think it's a good compromise between dev utility and prod security.
There was a problem hiding this comment.
Is the correct thing to do here to extend the container via a test specific Dockerfile adding testing related tools? We could either pre-build that "enhanced" container or do it on the fly I would think.
There was a problem hiding this comment.
test specific
Isn't everything we are doing test-specific? Are there plans to start using Docker in production?
There was a problem hiding this comment.
@clintonb If we start by keeping test and potentially production separate, we maintain the flexibility of being able to use docker for production in the future, pretty cheaply. It's generally much harder to separate test and prod after the fact if we do decide we want to use docker in production.
@e0d my thought was that for adding a few more apt packages, we can just do it via the current overrides file which I understood to be test specific ansible overrides.
There was a problem hiding this comment.
@clintonb I think the answers are no and no in that order ;)
Even without specific plans, I don't see why we wouldn't establish a solid pattern for separation of concerns, especially given the facility of layering changes on containers available in docker.
There was a problem hiding this comment.
|
|
||
| cd /edx/app/ecommerce/ecommerce | ||
|
|
||
| pip install -U pip wheel |
There was a problem hiding this comment.
It would be nice to install these in the image.
There was a problem hiding this comment.
Waiting on responses to the question above which will likely inform how we deal with this as well.
There was a problem hiding this comment.
I believe pip is already installed as a part of the standard requirements, this bit was sort of cargo-culted, I think it makes more sense to just use the version that is in the container, objections to just removing this line?
There was a problem hiding this comment.
pip is installed; but, it's usually an older version (7.x) instead of the latest (9.x). I haven't noticed any performance improvements; but, pip will show an upgrade warning every time it is invoked. wheel definitely leads to decreased install times, and should be included in the image.
There was a problem hiding this comment.
Thinking more about this, the correct way to do this would be to actualy upgard the pip in the container that is installed via ansible. As for wheel, why no install this as a part of 'make requirements' which is dev specific anyway?
There was a problem hiding this comment.
Also, upon further investigation, virtualenv has been installing wheel by default for a while now: https://virtualenv.pypa.io/en/latest/changes/#id19 If this is not working, then it is probably a bug in ecommerce. I quickly checked edxapp on a sandbox and it does have wheel installed and we don't do anything special there. I'd like to remove this line unless you have further objections @clintonb
There was a problem hiding this comment.
(facepalm) This line was pulled from .travis.yml. Travis, to my knowledge, doesn't use virtualenvs, hence our adding this line. If we are running tests in containers, this is not needed.
There was a problem hiding this comment.
So, is this necessary? Sounds like no, if we're using virtualenvs to run tests now. Though I don't see a virtualenv being sourced anywhere.
There was a problem hiding this comment.
@rlucioni you can see the venv being sourced at the top of run_tests.sh and run_coverage.sh
I'll remove this.
| pip install -U pip wheel | ||
| # Make it so bower can run without sudo. | ||
| # https://github.com/GeoNode/geonode/pull/1070 | ||
| echo '{ "allow_root": true }' > /root/.bowerrc |
There was a problem hiding this comment.
This would also be useful in the image.
There was a problem hiding this comment.
Waiting on responses to the question above which will likely inform how we deal with this as well.
There was a problem hiding this comment.
I think adding this to the Dockerfile directly makes sense since it's specific to running bower in docker.
There was a problem hiding this comment.
This needs to be left as is because adding it to the root Dockerfile has the same issue as adding other test related config to it. This is not needed for the basic build because that is run by ansible and runs as the ecommerce user. The trouble is that we are running these tests using volume mounting which has various permissions issues when we try to use it in conjunction with ansible.
feanil
force-pushed
the
feanil/ecommerce_docker_testing
branch
from
f7b53d7
to
c9beeb2
Compare
This allows us to further test that the build on the correct version of the correct OS.
The VENv has the correct version of pip(same as in production), and installs wheel by default.
feanil
force-pushed
the
feanil/ecommerce_docker_testing
branch
from
621936e
to
f194e2b
Compare
This allows us to further test that the build on the correct version of the correct OS.