hyperledger / fabric-samples

Samples for Hyperledger Fabric
https://wiki.hyperledger.org/display/fabric
Apache License 2.0
2.78k stars 3.37k forks source link

Decouple fabric tools image from fabric sample #1186

Closed SamYuan1990 closed 5 months ago

SamYuan1990 commented 6 months ago

Decouple fabric tools image from fabric sample. change list for running on github action:

The current PR seems has many code smells... but it seems works.

SamYuan1990 commented 6 months ago
Creating config transaction to add org3 to network
Using organization 1
++ peer channel fetch config config_block.pb -o localhost:7050 --ordererTLSHostnameOverride orderer.example.com -c mychannel --tls --cafile /home/runner/work/fabric-samples/fabric-samples/test-network/addOrg3/organizations/ordererOrganizations/example.com/tlsca/tlsca.example.com-cert.pem
Fetching the most recent configuration block for the channel
2024-03-20 15:09:14.997 UTC 0001 ERRO [main] InitCmd -> Fatal error when initializing core config : error when reading core config file: Config File "core" Not Found in "[/home/runner/work/fabric-samples/fabric-samples/test-network/addOrg3]"

@denyeart , I am got confused a little, could you please help with which core config is missing here? when we try to add org3?

SamYuan1990 commented 6 months ago

@denyeart , please help retry failed test case and review this PR. :-)

SamYuan1990 commented 6 months ago

@SamYuan1990 Thank you for submitting the PR. I have some review comments:

  • I noticed the use of symbolic links for scripts and organizations folders, which might be confusing for beginners looking at the code. I believe it would be better to use relative paths instead of symbolic links. Would it be difficult to make this change?
  • Many of the modified files contain comments stating like "This script is designed to be run in the cli container," which should also be updated.
  • For org3, I think it's preferable to refer to core.yaml in fabric-samples/config, similar to org1 and 2, by setting the FABRIC_CFG_PATH instead of adding a new core file.
  • Additionally, the intermediate artifacts related to configtx that were previously generated within cli container, such as .json, .tx, and *.pb files, remain in the root of the test-network or addOrg3 directories. It might be better to generate these in a dedicated directory, like channel-artifacts, to keep things organized. Would this approach be feasible?
  • The Test Network SBE GitHub test seems to be failing due to a different issue unrelated to this PR, so it might be okay to ignore it for now.

@satota2, with my hands on, the most challenging for this PR is the loop of relative paths. for example/to be details: as https://github.com/hyperledger/fabric-samples/blob/main/test-network/compose/compose-test-net.yaml#L155 the docker compose provides a good beginning as the /scripts folder.

which makes https://github.com/hyperledger/fabric-samples/blob/main/test-network/scripts/org3-scripts/joinChannel.sh#L26 org3 related script starts from this absolute directory.

the symbolic links... provides the same folder structure, specific for the loop case. for example a loop like: https://github.com/hyperledger/fabric-samples/blob/main/test-network/scripts/org3-scripts/updateChannelConfig.sh#L27 https://github.com/hyperledger/fabric-samples/blob/main/test-network/scripts/configUpdate.sh#L9

as those scripts also reused by org1 and org2, if we just change the scripts for org3, which will break the relative paths for org1 and org2. hence, can we adjust org3's folder structure make it parallel with org1 and org2 to make all the orgs share the same behavior for every org, such as relative paths for shell, FABRIC_CFG_PATH and generate files?

satota2 commented 6 months ago

@satota2, with my hands on, the most challenging for this PR is the loop of relative paths. for example/to be details: as https://github.com/hyperledger/fabric-samples/blob/main/test-network/compose/compose-test-net.yaml#L155 the docker compose provides a good beginning as the /scripts folder.

which makes https://github.com/hyperledger/fabric-samples/blob/main/test-network/scripts/org3-scripts/joinChannel.sh#L26 org3 related script starts from this absolute directory.

the symbolic links... provides the same folder structure, specific for the loop case. for example a loop like: https://github.com/hyperledger/fabric-samples/blob/main/test-network/scripts/org3-scripts/updateChannelConfig.sh#L27 https://github.com/hyperledger/fabric-samples/blob/main/test-network/scripts/configUpdate.sh#L9

as those scripts also reused by org1 and org2, if we just change the scripts for org3, which will break the relative paths for org1 and org2. hence, can we adjust org3's folder structure make it parallel with org1 and org2 to make all the orgs share the same behavior for every org, such as relative paths for shell, FABRIC_CFG_PATH and generate files?

@SamYuan1990 Thank you for your response. I understand that this solution is challenging due to scripts being called from structurally different folders. However, I believe we should avoid using symbolic links as it may lead to confusion.

As an alternative, what about using absolute paths in each script instead of relative ones? For example, we could use dirname $0 in each script to dynamically obtain the absolute path of the script itself and use that as a starting point. What are your thoughts on this approach?

SamYuan1990 commented 6 months ago

@satota2, with my hands on, the most challenging for this PR is the loop of relative paths. for example/to be details: as https://github.com/hyperledger/fabric-samples/blob/main/test-network/compose/compose-test-net.yaml#L155 the docker compose provides a good beginning as the /scripts folder. which makes https://github.com/hyperledger/fabric-samples/blob/main/test-network/scripts/org3-scripts/joinChannel.sh#L26 org3 related script starts from this absolute directory. the symbolic links... provides the same folder structure, specific for the loop case. for example a loop like: https://github.com/hyperledger/fabric-samples/blob/main/test-network/scripts/org3-scripts/updateChannelConfig.sh#L27 https://github.com/hyperledger/fabric-samples/blob/main/test-network/scripts/configUpdate.sh#L9 as those scripts also reused by org1 and org2, if we just change the scripts for org3, which will break the relative paths for org1 and org2. hence, can we adjust org3's folder structure make it parallel with org1 and org2 to make all the orgs share the same behavior for every org, such as relative paths for shell, FABRIC_CFG_PATH and generate files?

@SamYuan1990 Thank you for your response. I understand that this solution is challenging due to scripts being called from structurally different folders. However, I believe we should avoid using symbolic links as it may lead to confusion.

As an alternative, what about using absolute paths in each script instead of relative ones? For example, we could use dirname $0 in each script to dynamically obtain the absolute path of the script itself and use that as a starting point. What are your thoughts on this approach?

I see, let's try.

SamYuan1990 commented 6 months ago
2024-03-30 08:37:06.594 UTC 0001 ERRO [main] InitCmd -> Cannot run peer because cannot init crypto, specified path "/home/runner/work/fabric-samples/fabric-samples/test-network/addOrg3/organizations/peerOrganizations/org1.example.com/users/Admin@org1.example.com/msp" does not exist or cannot be accessed: stat /home/runner/work/fabric-samples/fabric-samples/test-network/addOrg3/organizations/peerOrganizations/org1.example.com/users/Admin@org1.example.com/msp: no such file or directory

we can't simply using FABRIC_CFG_PATH=$PWD/../../config/ ... otherwise the msp folder will be wrong.

SamYuan1990 commented 6 months ago

@satota2, with my hands on, the most challenging for this PR is the loop of relative paths. for example/to be details: as https://github.com/hyperledger/fabric-samples/blob/main/test-network/compose/compose-test-net.yaml#L155 the docker compose provides a good beginning as the /scripts folder. which makes https://github.com/hyperledger/fabric-samples/blob/main/test-network/scripts/org3-scripts/joinChannel.sh#L26 org3 related script starts from this absolute directory. the symbolic links... provides the same folder structure, specific for the loop case. for example a loop like: https://github.com/hyperledger/fabric-samples/blob/main/test-network/scripts/org3-scripts/updateChannelConfig.sh#L27 https://github.com/hyperledger/fabric-samples/blob/main/test-network/scripts/configUpdate.sh#L9 as those scripts also reused by org1 and org2, if we just change the scripts for org3, which will break the relative paths for org1 and org2. hence, can we adjust org3's folder structure make it parallel with org1 and org2 to make all the orgs share the same behavior for every org, such as relative paths for shell, FABRIC_CFG_PATH and generate files?

@SamYuan1990 Thank you for your response. I understand that this solution is challenging due to scripts being called from structurally different folders. However, I believe we should avoid using symbolic links as it may lead to confusion. As an alternative, what about using absolute paths in each script instead of relative ones? For example, we could use dirname $0 in each script to dynamically obtain the absolute path of the script itself and use that as a starting point. What are your thoughts on this approach?

I see, let's try.

@satota2 , I suppose I found a way to make it by a env named as test_network_home please help review this PR. @denyeart , once this PR been merged, which means we decoupled fabric tools from fabric sample, then we can deprecated fabric tools image.

SamYuan1990 commented 6 months ago

@SamYuan1990, Cc: @denyeart

Thank you for the update. The code has been refined, and I was able to verify that Org3 can now be added in the local environment as well. However, it appears that the following improvements I previously suggested have not yet been incorporated. Would it be possible for you to address these areas? We might consider merging this PR as it stands and then either you, I, or someone else could quickly create a new PR to tackle these areas for further code cleanup. How does that sound?

Comments for improvements I previously suggested:

  • I think that using uppercase for the test_network_home environment variable would be preferable since it is a convention for environment variables.
  • For org3, it seems better to refer to core.yaml in fabric-samples/config, similar to org1 and 2, by setting the FABRIC_CFG_PATH instead of adding a new core file.
  • The intermediate artifacts related to configtx that were previously generated within the cli container, such as .json, .tx, and *.pb files, remain in the root of the test-network or addOrg3 directories. It might be more organized to generate these in a dedicated directory, like channel-artifacts, to keep things tidy.

hi @satota2 , my goal to have this PR is to proof we can deprecated fabric tools image but not make a perfect change for multiple org solution. Hence the enhancement or totally refactor can be in other PRs.

SamYuan1990 commented 6 months ago

@SamYuan1990 Cc: @denyeart

hi @satota2 , my goal to have this PR is to proof we can deprecated fabric tools image but not make a perfect change for multiple org solution. Hence the enhancement or totally refactor can be in other PRs.

Thank you for your response. We'll mark the tasks up to this point as completed within this PR and do the refactoring and improvements in a new PR. Thanks for all your hard work so far.

Would you be interested in handling the refactoring? Your involvement would be welcome. However, if you prefer not to, that's perfectly fine. Either I or someone else can take it from there.

Hi @satota2 , you can try with refactor and I would like to learn from it.

satota2 commented 6 months ago

Hi @SamYuan1990,

Hi @satota2 , you can try with refactor and I would like to learn from it. OK! I will attempt the refactoring once we have merged this PR. Thank you once again for the hard work!

satota2 commented 5 months ago

Hi @SamYuan1990,

Hi @satota2 , you can try with refactor and I would like to learn from it.

OK! I will attempt the refactoring once we have merged this PR. Thank you once again for the hard work!

I've merged the PR and have now submitted a new PR (#1193) for the refactoring as discussed. Please take a look when you have a moment.

Hi @denyeart Once the new PR (#1193) is merged, I think that the tasks related to removing fabric-tools on the fabric-samples side will be completed.