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 portmap for docker agent #255

Merged
merged 1 commit into from
Jun 17, 2021
Merged

Conversation

dexhunter
Copy link
Contributor

Signed-off-by: Dixing Xu dixingxu@gmail.com

* related to hyperledger#254
* add ports options when creating docker agent

Signed-off-by: Dixing Xu <dixingxu@gmail.com>
@dexhunter dexhunter requested review from a team, yeasy and XuHugo June 13, 2021 14:40
@@ -43,7 +43,7 @@ def create_node():

try:
# same as `docker run -dit yeasy/hyperledge-fabric:2.2.0 -e VARIABLES``
container = client.containers.run(request.form.get('img'), request.form.get('cmd'), detach=True, tty=True, stdin_open=True, name=request.form.get('name'), environment=env)
container = client.containers.run(request.form.get('img'), request.form.get('cmd'), detach=True, tty=True, stdin_open=True, name=request.form.get('name'), environment=env, ports=request.form.get('port_map'))
Copy link
Member

Choose a reason for hiding this comment

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

And the port mapping should be sent to the docker daemon to take effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure if I understand the question correctly. From what I read on the doc, we pass port-map as a mapping dict from public to internal services, and this ports(*dict*) parameter binds the ports insides the container as listed in this doc . For example, we can pass {'7053/tcp': 8053} which will expose port 7053 inside the container as port 8053 on the host.

Or am I understanding this feature wrong? Do you want to use port-map to configure the corresponding blockchain services? Maybe we can change that on the config-file?

Copy link
Member

Choose a reason for hiding this comment

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

Same understanding here. @XuHugo comments?

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 what he understands is correct. The original port of the node (defined in config. Yaml) does not change. Let's map it to the port of the host;

Copy link
Member

Choose a reason for hiding this comment

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

In this case, we still need to specify the host port via the docker client? Or this patchset is enough?

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 patchset is enough.

@dexhunter dexhunter requested a review from yeasy June 17, 2021 08:05
@yeasy yeasy merged commit 1c20421 into hyperledger:master Jun 17, 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.

3 participants