chore: add Dockerfile to run tests #46

poppoerika · 2022-11-01T19:30:35Z

Also added instructions to build an image and run the tests in a container in both regular README and Japanese version.

Closes #27

cprice404 · 2022-11-01T19:34:25Z

Dockerfile

+ENV TEST_AUTH_TOKEN=$token
+ENV TEST_CACHE_NAME="php-integration-test-cache"
+
+CMD ["php", "vendor/phpunit/phpunit/phpunit", "--configuration ", "phpunit.xml"]


can we make this a little more general purpose? e.g. it could also be used for running examples. I think we could just remove this CMD and then users can pass it at the end of their "docker run" command?

That makes sense! Will update Dockerfile and README.

cprice404 · 2022-11-03T15:45:08Z

Dockerfile

+RUN cd /usr/local/bin && curl -sS https://getcomposer.org/installer | php
+RUN cd /usr/local/bin && mv composer.phar composer
+RUN pecl install grpc
+RUN docker-php-ext-enable grpc


Everything above this line seems great and is working very well for me.

I'd like to see if we can keep this Dockerfile generic, though, so I have some thoughts on the remaining lines.

I think that in the not-too-distant future (not as part of this ticket or PR), I'd like to publish this image to a public ECR repo, something like momento-php or momento-php-dev or something. Then we could re-use it across multiple projects, and devs could pull it without having to do the whole build (which is pretty expensive for the grpc part).

So with that in mind I think we might want to see if we can get rid of the remaining lines after this. I will add some additional comments below.

cprice404 · 2022-11-03T15:46:34Z

Dockerfile

+
+COPY . /app
+
+WORKDIR /app


If we copy the app dir into the docker image then we have to re-build it every time we change the app, and also it makes this docker image very tightly coupled to this repo.

Instead I think we can use -v flags when we call docker run, and that will allow us to mount the appropriate directories into the docker container. So I think we can get rid of these lines.

I think with Dockerfile it might be difficult to achieve this.
What I have tried during docker build:

Copy only composer.json and composer.lock

Run composer install

Results in creation of vender which includes all the packages' files

And at docker run, set a volume to mount client-sdk-php to a Docker container.
But what happens is that this volume overwrites vendor and now there are no packages installed.

It looks like we could specify which directories/files to mount using docker-compose.yml. But I don't think that achieve what we're trying to do, which re-using this image for other purposes.

okay, I will pull this down and look into it when i get a chance. It might be tomorrow.

I suspect we can just mount the vendor directory to keep the image generic but I need to double check to make sure it doesn't contain any native extensions or anything like that.

cprice404 · 2022-11-03T15:50:05Z

Dockerfile

+
+WORKDIR /app
+
+RUN composer install


This part is obviously important and I can imagine that maybe it is expensive?

Do you know where composer installs files to? Are they installed somewhere underneath the current working directory? Or in a centralized cache that can be re-used by multiple PHP apps on the host?

I'm wondering if we can just have a generic composer.json file that we burn into the image, which would install all of our normal deps and maybe laravel too, and use composer install on that file in order to bake all of those deps into the generic image. And then they could be re-used for various different apps that we might want to test with. I'm not sure if that's possible or not. Do you know?

I don't think baking cache into a image is not possible but we can cache composer packages for the first build and subsequent builds can export data from that cache.
I think we can use this.
I'd assume we will have build/push actions on GitHub action so maybe we're interested in using this?
Also, we can make it so that only new packages are installed in the subsequent builds by copying composer.json and composer.lock first and then run composer install.

Please let me know what you think!

Don't worry about the automated build/push for now, we can tackle that a little later. but thanks for thinking about it.

As for the caching of composer stuff via that first link you sent, it might be good for us to have a zoom to brainstorm on the ideas there.

The idea of copying in the composer files and then doing composer install is definitely along the lines of what I was thinking.

cprice404 · 2022-11-03T15:50:50Z

Dockerfile

+ENV TEST_AUTH_TOKEN=$token
+ENV TEST_CACHE_NAME="php-integration-test-cache"
+ENV MOMENTO_AUTH_TOKEN=$token
+ENV CACHE_NAME=$cache_name


Again in the spirit of trying to make the image as re-usable as possible, and avoid the need to re-build it, I would get rid of all of these. They can be passed in as env vars via -e flag to docker run.

I will do this.

cprice404 · 2022-11-03T15:53:35Z

README.template.md

@@ -59,4 +59,12 @@ Coming soon!

 Coming soon!

+### Running Tests


These instructions are relevant to Momento devs but probably not to most OSS users. So we should create a CONTRIBUTING.md file and put them in there instead of in the main README.

Take a look at the other SDK repos and see if we have an example of a CONTRIBUTING.md anywhere yet. Ultimately we will want to have one in every SDK repo and we will want to make them somewhat consistent, but I'm not sure if we've really established a pattern for that yet. If not, you can just get us started :)

I would recommend having two different sections in the CONTRIBUTING.md: one for how to build the docker image, and one for how to run things. Our goal will be for devs to only need to build the docker image once (or very rarely), and then they should only need to do variations on docker run once they have the image.

cprice404 · 2022-11-03T15:55:57Z

README.template.md

+```bash
+export DOCKER_COMMAND="php vendor/phpunit/phpunit/phpunit --configuration phpunit.xml"
+docker build --tag php-test --build-arg token=<YOUR_AUTH_TOKEN> .
+docker run -d -it php-test bash -c $DOCKER_COMMAND


This didn't work for me; to get it to work I ran this command:

docker run -it php-test php vendor/phpunit/phpunit/phpunit --configuration phpunit.xml

When you follow my advice above about making the build more generic, you are going to end up with some additional args that are required for the docker run command; e.g. some -es to set env vars, and probably a -v to mount the sdk code. At that point the docker run command will be fairly long so it will probably be a good idea to have some bash scripts that people can use. e.g. dev-docker-build.sh, and dev-run-integration-tests.sh or something like that.

cprice404 · 2022-11-03T15:56:14Z

src/Cache/_ScsDataClient.php

@@ -521,7 +520,6 @@ public function dictionarySetBatch(string $cacheName, string $dictionaryName, ar
            validateCacheName($cacheName);
            validateDictionaryName($dictionaryName);
            validateItems($items);
-            validateFieldsKeys($items);


just curious what the deal with this one was

Hmm sorry I thought I added it back after trying to figure out why it failed. I'm adding this back.

… on docker; update README; remove env variables from Dockerfile

…/dockerize-php-test

ove ENV from Dockerfile

poppoerika · 2022-11-08T19:13:39Z

@cprice404 I think class mapping in composer.json is causing an issue to copy composer.json and composer.lock first and run composer install to build a Docker image.
I know this is one way to optimize Docker build but I'm leaving it as it is originally (copying the entire files and then run composer install).

cprice404

this looks good for now! i will play with the composer part later when i get a chance. thanks!

chore: add Dockerfile to run tests; update README template and JP README

f2b125e

poppoerika requested review from pgautier404, cprice404 and kvcache November 1, 2022 19:30

cprice404 reviewed Nov 1, 2022

View reviewed changes

poppoerika added 2 commits November 1, 2022 14:35

fix: make docker image more flexible and pass command via docker run

d9c0efb

fix: remove wrong data validation for setAddBatch

e797d5d

poppoerika requested a review from cprice404 November 1, 2022 22:09

cprice404 reviewed Nov 3, 2022

View reviewed changes

poppoerika added 5 commits November 3, 2022 15:20

chore: add CONTRIBUTING.md; add shell script to run integration tests…

d120360

… on docker; update README; remove env variables from Dockerfile

chore: update layer order

9a84329

Merge branch 'main' of github.com:momentohq/client-sdk-php into chore…

862285b

…/dockerize-php-test

chore: remove ENV from Dockerfile

bc42e5d

chore: rem

f6e97b5

ove ENV from Dockerfile

poppoerika requested a review from cprice404 November 8, 2022 19:10

cprice404 approved these changes Nov 8, 2022

View reviewed changes

poppoerika merged commit 7385920 into main Nov 8, 2022

malandis deleted the chore/dockerize-php-test branch November 1, 2024 21:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add Dockerfile to run tests #46

chore: add Dockerfile to run tests #46

poppoerika commented Nov 1, 2022

cprice404 Nov 1, 2022

poppoerika Nov 1, 2022

cprice404 Nov 3, 2022

cprice404 Nov 3, 2022

poppoerika Nov 3, 2022 •

edited

Loading

cprice404 Nov 7, 2022

cprice404 Nov 3, 2022

poppoerika Nov 3, 2022

cprice404 Nov 3, 2022

cprice404 Nov 3, 2022

poppoerika Nov 3, 2022

cprice404 Nov 3, 2022

cprice404 Nov 3, 2022

cprice404 Nov 3, 2022

poppoerika Nov 3, 2022

poppoerika commented Nov 8, 2022

cprice404 left a comment

		@@ -59,4 +59,12 @@ Coming soon!

		Coming soon!

		### Running Tests


		COPY . /app

		WORKDIR /app

chore: add Dockerfile to run tests #46

chore: add Dockerfile to run tests #46

Conversation

poppoerika commented Nov 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poppoerika Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poppoerika commented Nov 8, 2022

cprice404 left a comment

Choose a reason for hiding this comment

poppoerika Nov 3, 2022 •

edited

Loading