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

Completely Remove Short Name Support #390

Merged
merged 2 commits into from
Dec 22, 2020

Conversation

jcastrence
Copy link
Contributor

Type of change

  • Improvement (improvement to code, performance, etc)

Description

  • Short names are a crutch that can prevent someone from learning how to deploy their own chaincode. Short names have been replaced with their full path name. Short names are no longer recognized by shell scripts.

@jcastrence jcastrence requested a review from a team as a code owner December 11, 2020 02:24
nikhil550
nikhil550 previously approved these changes Dec 11, 2020
test-network/scripts/deployCC.sh Show resolved Hide resolved
ci/scripts/run-test-network-basic.sh Outdated Show resolved Hide resolved
test-network/scripts/deployCC.sh Outdated Show resolved Hide resolved
test-network/scripts/deployCC.sh Outdated Show resolved Hide resolved
@jcastrence jcastrence force-pushed the remove_short_names_2 branch 2 times, most recently from 904f273 to 86eea66 Compare December 16, 2020 14:10
@nikhil550
Copy link
Contributor

I was playing around with the error text,a nd I realized that the defaults are still set in the main network.sh script:

# chaincode name defaults to "basic"
CC_NAME="basic"
# chaincode path defaults to "NA"
CC_SRC_PATH="NA"
# use go as the default language for chaincode
CC_SRC_LANGUAGE="go"

I would remove those from that script, and then set the defaults to "NA" in the first lines of the deployCC script. You can then set the following if statements accordingly

@jcastrence
Copy link
Contributor Author

@nikhil550 I noticed that when I was playing with the script but couldn't figure out why it was still setting them to the defaults. Thanks for pointing that out.

@nikhil550
Copy link
Contributor

I was playing around with the error text,a nd I realized that the defaults are still set in the main network.sh script:

# chaincode name defaults to "basic"
CC_NAME="basic"
# chaincode path defaults to "NA"
CC_SRC_PATH="NA"
# use go as the default language for chaincode
CC_SRC_LANGUAGE="go"

I would remove those from that script, and then set the defaults to "NA" in the first lines of the deployCC script. You can then set the following if statements accordingly

Actually, playing around, removing them is the wrong thing to do. If you remove the defaults, then the variables do not exist. Rather, set them to NA in the network.sh script instead.

@jcastrence
Copy link
Contributor Author

I was playing around with the error text,a nd I realized that the defaults are still set in the main network.sh script:

# chaincode name defaults to "basic"
CC_NAME="basic"
# chaincode path defaults to "NA"
CC_SRC_PATH="NA"
# use go as the default language for chaincode
CC_SRC_LANGUAGE="go"

I would remove those from that script, and then set the defaults to "NA" in the first lines of the deployCC script. You can then set the following if statements accordingly

Actually, playing around, removing them is the wrong thing to do. If you remove the defaults, then the variables do not exist. Rather, set them to NA in the network.sh script instead.

Would we still need to check if the variables are empty then in the deployCC script, since our variables will now always be set to NA by default?

@nikhil550
Copy link
Contributor

I was playing around with the error text,a nd I realized that the defaults are still set in the main network.sh script:

# chaincode name defaults to "basic"
CC_NAME="basic"
# chaincode path defaults to "NA"
CC_SRC_PATH="NA"
# use go as the default language for chaincode
CC_SRC_LANGUAGE="go"

I would remove those from that script, and then set the defaults to "NA" in the first lines of the deployCC script. You can then set the following if statements accordingly

Actually, playing around, removing them is the wrong thing to do. If you remove the defaults, then the variables do not exist. Rather, set them to NA in the network.sh script instead.

Would we still need to check if the variables are empty then in the deployCC script, since our variables will now always be set to NA by default?

You can check for "NA". see comment above.

Copy link
Contributor

@denyeart denyeart left a comment

Choose a reason for hiding this comment

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

I grepped fabric-samples and fabric for occurrences of "deployCC". I see a few more occurrences where not all three flags are provided (ccn, ccp, ccl).

test-network/scripts/deployCC.sh Outdated Show resolved Hide resolved
test-network/scripts/deployCC.sh Outdated Show resolved Hide resolved
test-network/scripts/deployCC.sh Outdated Show resolved Hide resolved
test-network/scripts/deployCC.sh Outdated Show resolved Hide resolved
elif [ ! -d "$CC_SRC_PATH" ]; then
fatalln "Path to chaincode does not exist. Please provide different path"
fatalln "Path to chaincode does not exist. Please provide different path. ./network.sh deployCC -ccn basic -ccp ../asset-transfer-basic/chaincode-go -ccl go"
Copy link
Contributor

Choose a reason for hiding this comment

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

You can probably remove the valid call example from this message. (If you keep it, add the text Valid call example:

@nikhil550
Copy link
Contributor

Looks good, just a couple nits.

You will need to update the help text to state that go is no longer he default chaincode language:

-ccl <language> - Programming language of the chaincode to deploy: go (default), java, javascript, typescript

Signed-off-by: Julian Castrence <juliancastrence@ibm.com>
Copy link
Contributor

@nikhil550 nikhil550 left a comment

Choose a reason for hiding this comment

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

LGTM. I will leave open in case anyone else has any comments

@nikhil550 nikhil550 merged commit 27ac653 into hyperledger:master Dec 22, 2020
@SamYuan1990
Copy link
Contributor

Hi guys,

Please correct me for any mis understanding.
In this pr, we are removed short names and replaced by full name. as

Short names are a crutch that can prevent someone from learning how to deploy their own chaincode. Short names have been replaced with their full path name. Short names are no longer recognized by shell scripts.

but ... why removed the default values?
is the default values prevent someone from learning or the short name prevents?

SamYuan1990 added a commit to SamYuan1990/tape that referenced this pull request Dec 25, 2020
Signed-off-by: SamYuan1990 <yy19902439@126.com>
SamYuan1990 added a commit to SamYuan1990/Probe that referenced this pull request Dec 25, 2020
Signed-off-by: SamYuan1990 <yy19902439@126.com>
guoger pushed a commit to Hyperledger-TWGC/tape that referenced this pull request Dec 29, 2020
Signed-off-by: SamYuan1990 <yy19902439@126.com>
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.

None yet

6 participants