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

Improve BFT sample on test-network #1083

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

satota2
Copy link
Contributor

@satota2 satota2 commented Aug 25, 2023

This patch includes:

  • Fixed a minor bug in the parsing of the BFT flag
  • Update of printHelp for use of the BFT flag
  • Fixed explanations

@satota2 satota2 requested a review from a team as a code owner August 25, 2023 03:57
Copy link
Contributor

@lehors lehors left a comment

Choose a reason for hiding this comment

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

There is a contradiction in the information being added. Ideally we would add code to prevent users from specifying both -ca and -bft if that combination isn't currently supported although this could be done in a different PR.

Comment on lines 28 to 29
println " \033[0;32mup\033[0m -ca -bft -r -d -s -verbose"
println " \033[0;32mup createChannel\033[0m -ca -bft -c -r -d -s -verbose"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is in contradiction with the note added "that currently this sample does not yet support the use of consensus type BFT and CA together", isn't it?

Comment on lines 134 to 139
println " \033[0;32mup createChannel\033[0m -ca -bft -c -r -d -s -verbose"
println " \033[0;32mcreateChannel\033[0m -bft -c -r -d -verbose"
Copy link
Contributor

Choose a reason for hiding this comment

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

same here.

@satota2
Copy link
Contributor Author

satota2 commented Aug 28, 2023

@lehors Thank you for your reviews.

I fixed the PRs based on your comments and submited it.
Please take the time to review it.

add code to prevent users from specifying both -ca and -bft if that combination isn't currently supported

Since there are cases where up command is executed separately, I have added check logic to prevent createChannel using BFT orderer on CA-enabled Fabric network.

Comment on lines +97 to +100
## User attempts to use BFT orderer in Fabric network with CA
if [ $BFT -eq 1 ] && [ -d "organizations/fabric-ca/ordererOrg/msp" ]; then
fatalln "Fabric network seems to be using CA. This sample does not yet support the use of consensus type BFT and CA together."
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

This test should be done after parsing the command line arguments so that it is also triggered if one simply does: "./network.sh up -ca -bft"

This patch includes:
- Fixed a minor bug in the parsing of the BFT flag
- Added check for when a user attempts to use BFT orderer in
  Fabric network with CA
- Update of printHelp for use of the BFT flag
- Fixed explanations

Signed-off-by: Tatsuya Sato <[email protected]>
@satota2
Copy link
Contributor Author

satota2 commented Aug 28, 2023

@lehors Thank you for your comments.
I newly add check logic executing after parsing the command line arguments.

The original logic is also retained to check the following case:

./network.sh up -ca
./network.sh createChannel -bft

Please confirm the updated PR.

Copy link
Contributor

@lehors lehors left a comment

Choose a reason for hiding this comment

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

That works. Thank you!

@lehors lehors merged commit 7d5aaf1 into hyperledger:main Aug 28, 2023
29 of 41 checks passed
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.

2 participants