Skip to content

doc: add How to test locally #222

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

Merged
merged 2 commits into from
Jul 11, 2022
Merged

doc: add How to test locally #222

merged 2 commits into from
Jul 11, 2022

Conversation

jiang1997
Copy link
Contributor

No description provided.

@wu-sheng wu-sheng requested a review from kezhenxu94 July 11, 2022 11:47
@wu-sheng wu-sheng added the documentation Improvements or additions to documentation label Jul 11, 2022
Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

Thank you @jiang1997 for adding this

Comment on lines 18 to 34
Create a file in the folder you just cloned.
```
FROM python:3.8

WORKDIR /usr/src/app

COPY . .

RUN pip install -r requirements.txt
RUN pip install -e .[test]
then
```

Then run the command below to create the docker image we need later. Because the test process runs in the docker container, we need to apply the changes you did to the docker image. You may need to run this command whenever you change any files before the test.
```
docker build -t apache/skywalking-python-agent:latest-plugin . -f Dockerfile.test
```
Copy link
Member

Choose a reason for hiding this comment

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

All these can be simplified as

docker build --build-arg BASE_PYTHON_IMAGE=3.6 -t apache/skywalking-python-agent:latest-plugin --no-cache . -f tests/plugin/Dockerfile.plugin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, when users have not run any make command, and there is no venv folder, this one-line command works as expected.
I observed when there is venv folder in ROOT, this one-line command runs into error as below.

Step 6/7 : RUN cd /agent && make setup-test
 ---> Running in 8c58f3791fcb
venv/bin/python -m pip install grpcio --ignore-installed
/agent/venv/bin/python: No module named pip
make: *** [Makefile:33: setup] Error 1
The command '/bin/sh -c cd /agent && make setup-test' returned a non-zero code: 2

I guess because the existed venv is not configured for the environment inside docker image, but make script runs inside docker image do not aware of it.

After changing tests/plugin/Dockerfile.plugin as below, everything works fine.

ARG BASE_PYTHON_IMAGE

FROM python:${BASE_PYTHON_IMAGE}

ARG ROOT=.

WORKDIR /agent

ADD $ROOT /agent

RUN cd /agent && rm -rf venv && make setup-test

ENV PATH="/agent/venv/bin:$PATH"

Copy link
Member

Choose a reason for hiding this comment

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

Hi @jiang1997 thanks for pointing this out. I think you're right. We have another more canonical way to avoid this. Can you please add a file .dockerignore in root directory of this project and add the venv as its content? This should resolve the problem.

File .dockerignore:

venv

Copy link
Member

Choose a reason for hiding this comment

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

Don't forget to add license header for the new file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright

Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

Thanks @jiang1997 !!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants