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

Add dockerfile to agent-docker and Fix some bugs #192

Merged
merged 2 commits into from
Nov 18, 2020

Conversation

XuHugo
Copy link
Contributor

@XuHugo XuHugo commented Nov 5, 2020

Signed-off-by: XuHugo xq-310@163.com

If we use agent docker, we should replace from_ env() with dockerclient()。
When the image is generated, we do not know the IP address of the docker server; therefore, we need to pass in the IP address when running.

Signed-off-by: XuHugo <xq-310@163.com>
@XuHugo XuHugo requested a review from a team November 5, 2020 07:26
@XuHugo
Copy link
Contributor Author

XuHugo commented Nov 5, 2020

@yeasy @dexhunter

@yeasy yeasy requested a review from dexhunter November 7, 2020 02:07
@XuHugo XuHugo removed their assignment Nov 7, 2020

app = Flask(__name__)
PASS_CODE = 'OK'
FAIL_CODE = 'Fail'

client = docker.from_env()
docker_url = os.getenv("DOCKER_URL")
Copy link
Contributor

Choose a reason for hiding this comment

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

where is the default value of DOCKER_URL set? @XuHugo

@@ -0,0 +1,5 @@
[global]
index-url=http://mirrors.cloud.aliyuncs.com/pypi/simple/
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is not necessary as developers are in different regions.

@XuHugo
Copy link
Contributor Author

XuHugo commented Nov 9, 2020 via email

@dexhunter
Copy link
Contributor

there is no default value,when the user starts the container,this parameter is passed in。and then if you use from_env,failed to complie docker。 | | 许强 | | 邮箱:xq-310@163.com | 签名由 网易邮箱大师 定制 On 11/09/2020 12:10, Dixing (Dex) Xu wrote: @dexhunter commented on this pull request. In src/agent/docker-rest-agent/server.py:
app = Flask(name) PASS_CODE = 'OK' FAIL_CODE = 'Fail'
-client = docker.from_env() +docker_url = os.getenv("DOCKER_URL") where is the default value of DOCKER_URL set? @XuHugo In src/agent/docker-rest-agent/pip.conf:
@@ -0,0 +1,5 @@
+[global] +index-url=http://mirrors.cloud.aliyuncs.com/pypi/simple/ I think this is not necessary as developers are in different regions. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

I see. Since I was using it natively, I didn't consider the case of assembling into an image. Anyways, the CI failure is irrelevant to this change. I think you can restart the CI to see if the problem persists.

@XuHugo XuHugo requested a review from dexhunter November 11, 2020 01:41
gunicorn==20.0.4
zope.event==4.5.0
zope.interface==5.1.2
Copy link
Contributor

Choose a reason for hiding this comment

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

ok, I am wondering if all those packages are necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the production environment, gunicorn is generally used to deploy the flask service

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, I understand the usage of gunicorn but what I mean is that since you added multiple dependencies (3->22), are all those dependencies necessary? Well, all can keep for now, until some one is not used anymore. 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.

Not all of them are necessary. For example, jinja2 will also be installed when you want to install flask. the requirements.txt is produced automatically;If you feel that you don't have to, you can leave them out, and it won't have any effect.

@yeasy
Copy link
Contributor

yeasy commented Nov 11, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dexhunter
Copy link
Contributor

The error was reading https://api.bitbucket.org/2.0/repositories/ww/goautoneg?fields=scm: 404 Not Found from here

@XuHugo
Copy link
Contributor Author

XuHugo commented Nov 11, 2020

The error was reading https://api.bitbucket.org/2.0/repositories/ww/goautoneg?fields=scm: 404 Not Found from here
Do you have any suggestions? I can't find a solution

@XuHugo XuHugo requested a review from dexhunter November 11, 2020 11:50
@dexhunter
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 192 in repo hyperledger/cello

@dexhunter
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yeasy
Copy link
Contributor

yeasy commented Nov 17, 2020

@XuHugo @dexhunter
We can comment out the fabric-operator agent building in the CI, before @litong01 helps fix the issue.

dexhunter added a commit to dexhunter/cello that referenced this pull request Nov 17, 2020
Signed-off-by: Dixing (Dex) Xu <dixingxu@gmail.com>
dexhunter added a commit to dexhunter/cello that referenced this pull request Nov 17, 2020
Signed-off-by: Dixing (Dex) Xu <dixingxu@gmail.com>
@dexhunter
Copy link
Contributor

@yeasy Sure, can rebase to #193

@dexhunter
Copy link
Contributor

@XuHugo Please rebase to the master branch and force push again. Thanks!

Signed-off-by: XuHugo <xq-310@163.com>
@XuHugo
Copy link
Contributor Author

XuHugo commented Nov 17, 2020

@XuHugo Please rebase to the master branch and force push again. Thanks!

ok,I'll fix it

@yeasy yeasy merged commit dca6db9 into hyperledger-cello:master Nov 18, 2020
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.

3 participants