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

Update test-network chaincode container versions #1266

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

bestbeforetoday
Copy link
Member

@bestbeforetoday bestbeforetoday commented Nov 5, 2024

The test-network peer configuration specifies $(TWO_DIGIT_VERSION) as the tag for the Node and Java chaincode containers. For Fabric v3.0, this requests fabric-nodeenv:3.0 and fabric-javaenv:3.0 Docker images to host Node and Java chaincode respectively. These images do not exist, which causes deployment of Node and Java chaincode to fail when using Fabric v3.0. Fabric v3.0 continues to use fabric-nodeenv:2.5 and fabric-javaenv:2.5.

This change updates the test-network peer configuration to explicitly specify 2.5 as the Node and Java chaincode Docker image tags. This is (currently) the correct version for both Fabric v2.5 and Fabric v3.0.

Closes #1267

@bestbeforetoday bestbeforetoday marked this pull request as ready for review November 5, 2024 22:54
@bestbeforetoday bestbeforetoday requested a review from a team as a code owner November 5, 2024 22:54
@satota2
Copy link
Contributor

satota2 commented Nov 6, 2024

@bestbeforetoday
Thank you for the update.

I noticed that this update changes the indentation in the YAML files from 2 spaces to 4. Is there a specific reason for this change? Other files appear to be consistently using 2 spaces, so it might be better to align with that convention for consistency. What do you think?

@satota2 satota2 self-requested a review November 6, 2024 04:33
@bestbeforetoday
Copy link
Member Author

I think this PR changes the indentation from 4 spaces to the same 2 spaces used in other Yaml files.

@satota2
Copy link
Contributor

satota2 commented Nov 6, 2024

@bestbeforetoday
Oh, my apologies! I somehow misunderstood, even though you had already corrected it to 2 spaces. Please disregard my previous comment… I’m a bit embarrassed about the mix-up!

@bestbeforetoday
Copy link
Member Author

bestbeforetoday commented Nov 6, 2024

No problem! I didn't set out to change the whitespace intentionally. I just have my editor set to autoformat on save. If you are happy with the 2 space indentation, the changes are (much) easier to review if you set GitHub's diff view settings to hide whitespace.

Copy link
Contributor

@satota2 satota2 left a comment

Choose a reason for hiding this comment

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

@bestbeforetoday
The updates look good to me. I have corrected the indentation for some of the commented-out configuration values, so please review those adjustments.

test-network/addOrg3/compose/docker/peercfg/core.yaml Outdated Show resolved Hide resolved
Comment on lines 514 to 525
# Parameters on creating docker container.
# Container may be efficiently created using ipam & dns-server for cluster
# NetworkMode - sets the networking mode for the container. Supported
# standard values are: `host`(default),`bridge`,`ipvlan`,`none`.
# Dns - a list of DNS servers for the container to use.
# Note: `Privileged` `Binds` `Links` and `PortBindings` properties of
# Docker Host Config are not supported and will not be used if set.
# LogConfig - sets the logging driver (Type) and related options
# (Config) for Docker. For more info,
# https://docs.docker.com/engine/admin/logging/overview/
# Note: Set LogConfig using Environment Variables is not supported.
hostConfig:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Parameters on creating docker container.
# Container may be efficiently created using ipam & dns-server for cluster
# NetworkMode - sets the networking mode for the container. Supported
# standard values are: `host`(default),`bridge`,`ipvlan`,`none`.
# Dns - a list of DNS servers for the container to use.
# Note: `Privileged` `Binds` `Links` and `PortBindings` properties of
# Docker Host Config are not supported and will not be used if set.
# LogConfig - sets the logging driver (Type) and related options
# (Config) for Docker. For more info,
# https://docs.docker.com/engine/admin/logging/overview/
# Note: Set LogConfig using Environment Variables is not supported.
hostConfig:
# Parameters on creating docker container.
# Container may be efficiently created using ipam & dns-server for cluster
# NetworkMode - sets the networking mode for the container. Supported
# standard values are: `host`(default),`bridge`,`ipvlan`,`none`.
# Dns - a list of DNS servers for the container to use.
# Note: `Privileged` `Binds` `Links` and `PortBindings` properties of
# Docker Host Config are not supported and will not be used if set.
# LogConfig - sets the logging driver (Type) and related options
# (Config) for Docker. For more info,
# https://docs.docker.com/engine/admin/logging/overview/
# Note: Set LogConfig using Environment Variables is not supported.
hostConfig:

hostConfig:
NetworkMode: host
Dns:
# - 192.168.0.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# - 192.168.0.1
# - 192.168.0.1

Comment on lines 498 to 507
# settings for docker vms
# docker:
# tls:
# enabled: false
# ca:
# file: docker/ca.crt
# cert:
# file: docker/tls.crt
# key:
# file: docker/tls.key
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# settings for docker vms
# docker:
# tls:
# enabled: false
# ca:
# file: docker/ca.crt
# cert:
# file: docker/tls.crt
# key:
# file: docker/tls.key
# settings for docker vms
# docker:
# tls:
# enabled: false
# ca:
# file: docker/ca.crt
# cert:
# file: docker/tls.crt
# key:
# file: docker/tls.key

Comment on lines 524 to 533
# hostConfig:
# NetworkMode: host
# Dns:
# # - 192.168.0.1
# LogConfig:
# Type: json-file
# Config:
# max-size: "50m"
# max-file: "5"
# Memory: 2147483648
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# hostConfig:
# NetworkMode: host
# Dns:
# # - 192.168.0.1
# LogConfig:
# Type: json-file
# Config:
# max-size: "50m"
# max-file: "5"
# Memory: 2147483648
# hostConfig:
# NetworkMode: host
# Dns:
# # - 192.168.0.1
# LogConfig:
# Type: json-file
# Config:
# max-size: "50m"
# max-file: "5"
# Memory: 2147483648
Suggested change
# hostConfig:
# NetworkMode: host
# Dns:
# # - 192.168.0.1
# LogConfig:
# Type: json-file
# Config:
# max-size: "50m"
# max-file: "5"
# Memory: 2147483648
# hostConfig:
# NetworkMode: host
# Dns:
# # - 192.168.0.1
# LogConfig:
# Type: json-file
# Config:
# max-size: "50m"
# max-file: "5"
# Memory: 2147483648

Comment on lines 509 to 523
# Enables/disables the standard out/err from chaincode containers for
# debugging purposes
# attachStdout: false

# Parameters on creating docker container.
# Container may be efficiently created using ipam & dns-server for cluster
# NetworkMode - sets the networking mode for the container. Supported
# standard values are: `host`(default),`bridge`,`ipvlan`,`none`.
# Dns - a list of DNS servers for the container to use.
# Note: `Privileged` `Binds` `Links` and `PortBindings` properties of
# Docker Host Config are not supported and will not be used if set.
# LogConfig - sets the logging driver (Type) and related options
# (Config) for Docker. For more info,
# https://docs.docker.com/engine/admin/logging/overview/
# Note: Set LogConfig using Environment Variables is not supported.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Enables/disables the standard out/err from chaincode containers for
# debugging purposes
# attachStdout: false
# Parameters on creating docker container.
# Container may be efficiently created using ipam & dns-server for cluster
# NetworkMode - sets the networking mode for the container. Supported
# standard values are: `host`(default),`bridge`,`ipvlan`,`none`.
# Dns - a list of DNS servers for the container to use.
# Note: `Privileged` `Binds` `Links` and `PortBindings` properties of
# Docker Host Config are not supported and will not be used if set.
# LogConfig - sets the logging driver (Type) and related options
# (Config) for Docker. For more info,
# https://docs.docker.com/engine/admin/logging/overview/
# Note: Set LogConfig using Environment Variables is not supported.
# Enables/disables the standard out/err from chaincode containers for
# debugging purposes
# attachStdout:
# false
# Parameters on creating docker container.
# Container may be efficiently created using ipam & dns-server for cluster
# NetworkMode - sets the networking mode for the container. Supported
# standard values are: `host`(default),`bridge`,`ipvlan`,`none`.
# Dns - a list of DNS servers for the container to use.
# Note: `Privileged` `Binds` `Links` and `PortBindings` properties of
# Docker Host Config are not supported and will not be used if set.
# LogConfig - sets the logging driver (Type) and related options
# (Config) for Docker. For more info,
# https://docs.docker.com/engine/admin/logging/overview/
# Note: Set LogConfig using Environment Variables is not supported.

# If you utilize external chaincode builders and don't need the default Docker chaincode builder,
# the endpoint should be unconfigured so that the peer's Docker health checker doesn't get registered.
endpoint:
unix:///var/run/docker.sock

# settings for docker vms
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# settings for docker vms
# settings for docker vms

Comment on lines 509 to 524
# Enables/disables the standard out/err from chaincode containers for
# debugging purposes
attachStdout:
false

# Parameters on creating docker container.
# Container may be efficiently created using ipam & dns-server for cluster
# NetworkMode - sets the networking mode for the container. Supported
# standard values are: `host`(default),`bridge`,`ipvlan`,`none`.
# Dns - a list of DNS servers for the container to use.
# Note: `Privileged` `Binds` `Links` and `PortBindings` properties of
# Docker Host Config are not supported and will not be used if set.
# LogConfig - sets the logging driver (Type) and related options
# (Config) for Docker. For more info,
# https://docs.docker.com/engine/admin/logging/overview/
# Note: Set LogConfig using Environment Variables is not supported.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Enables/disables the standard out/err from chaincode containers for
# debugging purposes
attachStdout:
false
# Parameters on creating docker container.
# Container may be efficiently created using ipam & dns-server for cluster
# NetworkMode - sets the networking mode for the container. Supported
# standard values are: `host`(default),`bridge`,`ipvlan`,`none`.
# Dns - a list of DNS servers for the container to use.
# Note: `Privileged` `Binds` `Links` and `PortBindings` properties of
# Docker Host Config are not supported and will not be used if set.
# LogConfig - sets the logging driver (Type) and related options
# (Config) for Docker. For more info,
# https://docs.docker.com/engine/admin/logging/overview/
# Note: Set LogConfig using Environment Variables is not supported.
# Enables/disables the standard out/err from chaincode containers for
# debugging purposes
attachStdout:
false
# Parameters on creating docker container.
# Container may be efficiently created using ipam & dns-server for cluster
# NetworkMode - sets the networking mode for the container. Supported
# standard values are: `host`(default),`bridge`,`ipvlan`,`none`.
# Dns - a list of DNS servers for the container to use.
# Note: `Privileged` `Binds` `Links` and `PortBindings` properties of
# Docker Host Config are not supported and will not be used if set.
# LogConfig - sets the logging driver (Type) and related options
# (Config) for Docker. For more info,
# https://docs.docker.com/engine/admin/logging/overview/
# Note: Set LogConfig using Environment Variables is not supported.

Comment on lines 498 to 540
# settings for docker vms
# docker:
# tls:
# enabled: false
# ca:
# file: docker/ca.crt
# cert:
# file: docker/tls.crt
# key:
# file: docker/tls.key

# Enables/disables the standard out/err from chaincode containers for
# debugging purposes
# attachStdout: false

# Parameters on creating docker container.
# Container may be efficiently created using ipam & dns-server for cluster
# NetworkMode - sets the networking mode for the container. Supported
# standard values are: `host`(default),`bridge`,`ipvlan`,`none`.
# Dns - a list of DNS servers for the container to use.
# Note: `Privileged` `Binds` `Links` and `PortBindings` properties of
# Docker Host Config are not supported and will not be used if set.
# LogConfig - sets the logging driver (Type) and related options
# (Config) for Docker. For more info,
# https://docs.docker.com/engine/admin/logging/overview/
# Note: Set LogConfig using Environment Variables is not supported.
# hostConfig:
# NetworkMode: host
# Dns:
# # - 192.168.0.1
# LogConfig:
# Type: json-file
# Config:
# max-size: "50m"
# max-file: "5"
# Memory: 2147483648

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# settings for docker vms
# docker:
# tls:
# enabled: false
# ca:
# file: docker/ca.crt
# cert:
# file: docker/tls.crt
# key:
# file: docker/tls.key
# Enables/disables the standard out/err from chaincode containers for
# debugging purposes
# attachStdout: false
# Parameters on creating docker container.
# Container may be efficiently created using ipam & dns-server for cluster
# NetworkMode - sets the networking mode for the container. Supported
# standard values are: `host`(default),`bridge`,`ipvlan`,`none`.
# Dns - a list of DNS servers for the container to use.
# Note: `Privileged` `Binds` `Links` and `PortBindings` properties of
# Docker Host Config are not supported and will not be used if set.
# LogConfig - sets the logging driver (Type) and related options
# (Config) for Docker. For more info,
# https://docs.docker.com/engine/admin/logging/overview/
# Note: Set LogConfig using Environment Variables is not supported.
# hostConfig:
# NetworkMode: host
# Dns:
# # - 192.168.0.1
# LogConfig:
# Type: json-file
# Config:
# max-size: "50m"
# max-file: "5"
# Memory: 2147483648
# settings for docker vms
# docker:
# tls:
# enabled: false
# ca:
# file: docker/ca.crt
# cert:
# file: docker/tls.crt
# key:
# file: docker/tls.key
# Enables/disables the standard out/err from chaincode containers for
# debugging purposes
# attachStdout:
# false
# Parameters on creating docker container.
# Container may be efficiently created using ipam & dns-server for cluster
# NetworkMode - sets the networking mode for the container. Supported
# standard values are: `host`(default),`bridge`,`ipvlan`,`none`.
# Dns - a list of DNS servers for the container to use.
# Note: `Privileged` `Binds` `Links` and `PortBindings` properties of
# Docker Host Config are not supported and will not be used if set.
# LogConfig - sets the logging driver (Type) and related options
# (Config) for Docker. For more info,
# https://docs.docker.com/engine/admin/logging/overview/
# Note: Set LogConfig using Environment Variables is not supported.
# hostConfig:
# NetworkMode: host
# Dns:
# # - 192.168.0.1
# LogConfig:
# Type: json-file
# Config:
# max-size: "50m"
# max-file: "5"
# Memory: 2147483648

@satota2
Copy link
Contributor

satota2 commented Nov 6, 2024

@bestbeforetoday
Through this PR, I noticed that the original YAML file in the Fabric core repository also uses 4 spaces for indentation.
https://github.com/hyperledger/fabric/blob/main/sampleconfig/core.yaml

So, I plan to submit a separate PR to the Fabric core repository to change this to 2 spaces.
I believe that aligning the indentation between the sample configuration in Fabric core and the configuration in fabric-samples’ test network will be beneficial for future updates and for users when making comparisons.

@satota2
Copy link
Contributor

satota2 commented Nov 6, 2024

@bestbeforetoday

No problem! I didn't set out to change the whitespace intentionally. I just have my editor set to autoformat on save. If you are happy with the 2 space indentation, the changes are (much) easier to review if you set GitHub's diff view settings to hide whitespace.

Thank you for your comment! I just saw it, and I appreciate the suggestion—I wasn't aware of that option, so it's very helpful.

So, I plan to submit a separate PR to the Fabric core repository to change this to 2 spaces.

Using this option, the indentation differences may indeed be less noticeable when comparing with the original file. That said, I think I'll still go ahead and submit a PR to adjust the indentation in the original file, leaving it to the maintainers to decide.

The test-network peer configuration specifies $(TWO_DIGIT_VERSION) as
the tag for the Node and Java chaincode containers. For Fabric v3.0,
this requests fabric-nodeenv:3.0 and fabric-javaenv:3.0 Docker images to
host Node and Java chaincode respectively. These images do not exist,
which causes deployment of Node and Java chaincode to fail when using
Fabric v3.0. Fabric v3.0 continues to use fabric-nodeenv:2.5 and
fabric-javaenv:2.5.

This change updates the test-network peer configuration to explicitly
specify `2.5` as the Node and Java chaincode Docker image tags. This is
(currently) the correct version for both Fabric v2.5 and Fabric v3.0.

Signed-off-by: Mark S. Lewis <Mark.S.Lewis@outlook.com>
@bestbeforetoday
Copy link
Member Author

Thank you for taking the time to go through all the formatting. Unfortunately, applying them all can't be done in one go and they are a pain to work through in the GitHub diff editor - I lost my batch updates several times. I have updated this PR to make the minimal changes required to resolve the actual problem being addressed. Formatting can be tackled in a separate PR when I or somebody else has time.

@bestbeforetoday
Copy link
Member Author

Given that these files are very similar to the sampleconfig in the main Fabric repo, but are already slightly out-of-date compared to Fabric's version, I wonder if it is worth removing all the problematic commented out sections and comments. Instead, a comment at the top of each file could reference the Fabric sampleconfig. Entirely up to you though.

Copy link
Contributor

@satota2 satota2 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 for taking the time to go through all the formatting. Unfortunately, applying them all can't be done in one go and they are a pain to work through in the GitHub diff editor - I lost my batch updates several times. I have updated this PR to make the minimal changes required to resolve the actual problem being addressed. Formatting can be tackled in a separate PR when I or somebody else has time.

@bestbeforetoday
Understood! I’ve reviewed the newly updated commit, and there are no issues with the changes. Thank you for handling this.

@satota2 satota2 merged commit 8136eb4 into hyperledger:main Nov 6, 2024
38 checks passed
@bestbeforetoday bestbeforetoday deleted the node-javaenv branch November 6, 2024 10:40
@satota2
Copy link
Contributor

satota2 commented Nov 7, 2024

Given that these files are very similar to the sampleconfig in the main Fabric repo, but are already slightly out-of-date compared to Fabric's version, I wonder if it is worth removing all the problematic commented out sections and comments. Instead, a comment at the top of each file could reference the Fabric sampleconfig. Entirely up to you though.

Thank you for your comment. As noted below, I’ve submitted a patch to fix the indentation in the original files:
hyperledger/fabric#5050

Once this is merged, I think it would be a good approach to update the various files in test-network based on the original file, taking your suggestion into consideration as well.

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.

Cannot deploy Node or Java chaincode with Fabric v3
2 participants