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

Provide clear guidance for debugging Java chaincode as a service #684 #724

Merged
merged 14 commits into from
Apr 26, 2022
Merged

Provide clear guidance for debugging Java chaincode as a service #684 #724

merged 14 commits into from
Apr 26, 2022

Conversation

jkneubuh
Copy link
Contributor

READY FOR MERGE

(This PR replaces PR #684, which lingered for too long in a DRAFT / review status, spanned multiple approaches, and had merge conflicts with the mainline.)

This PR addresses the need for a simple, clear example that illustrates the use of Chaincode-as-a-Service to build and run Java chaincode on a local development environment.

In addition, the README includes a section that describes how users may employ a Docker loopback interface to connect from a remote Peer binary to a local port on the host OS. In this manner, chaincode may be run locally while attached to a debugger, with the peer running on a remote pod in Kubernetes.

In the first iteration of this PR, we introduced a new ccpackage/* folder to hold descriptive files that would be used to generate the chaincode-as-a-service package, AND help with the deployment to k8s:

  • ccpackage/metadata.json (defines the package label)
  • ccpackage/connection.json (defines the CC service URL for gRPC access by the peer)
  • ccpackage/ccaas.json (describes the CC name, used to construct the CC pods in k8s, and CC IMAGE)

After community review, the PR has been reflected to remove the /ccpackage/ folder, opting to dynamically construct the cc package based on conventions and additional CLI arguments. This has the benefit of applying directly to most (if not all) of the current sample code base, without introducing the crufty "ccaas" deployment descriptors for yet-another-deployment-technique-for-fabric.

A multi-phase Azure pipeline has been included to verify the overall approach for both golang and Java CCaaS asset transfer chaincode samples.

Some additional / loose ends that may warrant future PRs and improvements:

  • Additional testing on the host.docker.internal DNS alias is required (e.g. Win, Linux, revs of Docker, etc.)

  • The doc sets up a flow with three installations of the same chaincode, but does not delete the chaincode between steps. (How does one delete a chaincode, btw?)

  • file-based arguments must all be absolute paths.

…incode as a service with a local debugger.

Signed-off-by: Josh Kneubuhl <jkneubuh@us.ibm.com>
Signed-off-by: Josh Kneubuhl <jkneubuh@us.ibm.com>
Signed-off-by: Josh Kneubuhl <jkneubuh@us.ibm.com>
Signed-off-by: Josh Kneubuhl <jkneubuh@us.ibm.com>
Signed-off-by: Josh Kneubuhl <jkneubuh@us.ibm.com>
Signed-off-by: Josh Kneubuhl <jkneubuh@us.ibm.com>
Signed-off-by: Josh Kneubuhl <jkneubuh@us.ibm.com>
Signed-off-by: Josh Kneubuhl <jkneubuh@us.ibm.com>
Signed-off-by: Josh Kneubuhl <jkneubuh@us.ibm.com>
Signed-off-by: Josh Kneubuhl <jkneubuh@us.ibm.com>
Signed-off-by: Josh Kneubuhl <jkneubuh@us.ibm.com>
@jkneubuh jkneubuh requested a review from a team as a code owner April 25, 2022 13:10
@jkneubuh
Copy link
Contributor Author

cc: @timo-kang @jt-nti

Copy link
Member

@jt-nti jt-nti left a comment

Choose a reason for hiding this comment

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

Looks great- just a couple of minor comments

Copy link
Member

@mbwhite mbwhite left a comment

Choose a reason for hiding this comment

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

Just some comments, some from a naive users perspective.

asset-transfer-basic/chaincode-java/README.md Outdated Show resolved Hide resolved
asset-transfer-basic/chaincode-java/README.md Outdated Show resolved Hide resolved
asset-transfer-basic/chaincode-java/README.md Outdated Show resolved Hide resolved
asset-transfer-basic/chaincode-java/README.md Outdated Show resolved Hide resolved
asset-transfer-basic/chaincode-java/README.md Outdated Show resolved Hide resolved
network cc invoke asset-transfer-debug '{"Args":["InitLedger"]}'
network cc query asset-transfer-debug '{"Args":["ReadAsset","asset1"]}'
```

Copy link
Member

Choose a reason for hiding this comment

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

Worth highlighting the connection timeout ? That when stepping through the code you need to be mindful of the transaction timingout.

ci/scripts/run-k8s-test-network-basic.sh Show resolved Hide resolved
function absolute_path() {
local relative_path=$1

local abspath="$( cd "${relative_path}" && pwd )"
Copy link
Member

Choose a reason for hiding this comment

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

think the 'realpath' command will do this for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it would.

If you "just run Linux."

Copy link
Member

Choose a reason for hiding this comment

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

oh...

comment retracted.

Signed-off-by: Josh Kneubuhl <jkneubuh@us.ibm.com>
Signed-off-by: Josh Kneubuhl <jkneubuh@us.ibm.com>
@jkneubuh
Copy link
Contributor Author

/azp run

Lint failed in a typescript routine from this PR? Wut?

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

Signed-off-by: Josh Kneubuhl <jkneubuh@us.ibm.com>
@mbwhite mbwhite enabled auto-merge (squash) April 26, 2022 14:32
@mbwhite mbwhite self-requested a review April 26, 2022 14:32
Copy link
Member

@mbwhite mbwhite left a comment

Choose a reason for hiding this comment

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

👏

@mbwhite mbwhite merged commit eeb4e61 into hyperledger:main Apr 26, 2022
@jkneubuh
Copy link
Contributor Author

w00t w00t

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.

3 participants