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

first-network/scripts/*: Make Chaincode name configurable #118

Merged
merged 1 commit into from
Feb 23, 2020

Conversation

maniankara
Copy link

Signed-off-by: Anoop Vijayan Maniankara anoop@tuxera.com

Signed-off-by: Anoop Vijayan Maniankara <anoop@tuxera.com>
@maniankara
Copy link
Author

Hello guys, by parameterising the app name, makes this as very good helper/handy for package, install, approveformyorg, etc. of real applications.

@nikhil550
Copy link
Contributor

Hi @maniankara - Thanks for your input.

I would have two comments on this PR

  • We are moving away from the older first-network sample to the test network sample. Improvements should be directed away from the first network and put into the test network instead.
  • The install script (deployCC.sh in the test network) should only be used for testing, or as an example for first time users to learn about the process. Fabric users should use the peer commands to package and deploy their own chaincode, rather than relying on the scripts provided by the samples. There is a PR open for a new deployCC tutorial that you can use to learn more about the process: [FAB-17269] Create new deploy a Chaincode tutorial fabric#666

@maniankara
Copy link
Author

Thanks for your explanations @nikhil550.
Sure, based on your comments above, this PR becomes invalid.

Fabric users should use the peer commands to package and deploy their own chaincode, rather than relying on the scripts provided by the samples

About this, yes peer command/binary should be used. But if you look at it, due to the nature of the commands and params, it gets very much cumbersome even for an advance user to quickly test/deploy them. So definitely a wrapper/helper on top of those commands are needed. As this is a common problem, it should somehow exist within the Hyperledger umbrella itself.

@lehors
Copy link
Contributor

lehors commented Feb 23, 2020

@maniankara, @nikhil550 is right in saying that we're moving away from BYFN but your PR looks good to me and I'm happy to merge it. Thank you for your contribution.

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.

LGTM

@lehors lehors merged commit 9ef61e2 into hyperledger:master Feb 23, 2020
@lindluni
Copy link
Contributor

Did anyone test this, there are multiple regressions in it when running against the default and not specifying your own name (though that also had easily introducible problems).

You read the NAME off the third input to installChaincode but didn't update any of the calls to installChaincode, and you don't read the name variable at all in the packageChaincode step or pass it in, and it hasn't been placed in the global namespace.

Will need to revert this as all byfn functionality is broken

jyellick pushed a commit that referenced this pull request Feb 26, 2020
…)" (#131)

This reverts commit 9ef61e2.

Introduced regressions when running without explicitly specifying
a chaincode label

Signed-off-by: Brett Logan <brett.t.logan@ibm.com>
@maniankara
Copy link
Author

@btl5037 @jyellick and others. I feel ashamed that I had pushed partially working commits to here. Now I fixed more things and managed to test this. Now it works well with byfn.sh.

I can write some justifications as in 'why' this is needed:

  1. There is no easy wrapper to manage the channel/chaincode Lifecycle.
    With this PR, after sourcing the necessary vars, my idea was to do this for my projects inside cli container:
source scripts/utils.sh
# to package myapp chaincode
packageChaincode 1 0 1 myapp
# install myapp chaincode
installChaincode 0 1 myapp
# approve for org
approveForMyOrg 1 0 1 myapp
....

As you see its very handy to use and this is currently missing.

Though the recommendation is to use the bare binary like peer lifecycle chaincode <ARGS> this is very much cumbersome to get things moving fast with those params like tls, etc.

Other approach is to duplicate the scripts from samples all over the users/repositories. I think you get the problem here.

@lindluni
Copy link
Contributor

Just to be clear, its we who are at fault here, our processes should catch problems, completely not your fault.
I'll take your new PR for a test run this afternoon, and thank you for the contribution

@lehors
Copy link
Contributor

lehors commented Feb 27, 2020

Sorry, I casually merged this on my way out without testing, in an effort to welcome a contributor. My bad.

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.

4 participants