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

Update 'Using Private Data in Fabric' tutorial #1875

Merged
merged 1 commit into from
Sep 20, 2020

Conversation

denali49
Copy link
Contributor

Signed-off-by: Chris Gabriel chris_gabriel_98@yahoo.com

Type of change

  • Documentation update

Description

This tutorial update integrates the new fabric-samples private data asset transfer README into the existing "Using Private Data in Fabric" tutorial per the direction of the fabric-samples workgroup.

Copy link
Contributor

@ale-linux ale-linux left a comment

Choose a reason for hiding this comment

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

A couple of minor nits. @denyeart have you (or do you know whether any of the maintainers have) tested the tutorial?

docs/source/private_data_tutorial.rst Outdated Show resolved Hide resolved
docs/source/private_data_tutorial.rst Outdated Show resolved Hide resolved
docs/source/private_data_tutorial.rst Outdated Show resolved Hide resolved
docs/source/private_data_tutorial.rst Outdated Show resolved Hide resolved
Copy link

@kalid117658 kalid117658 left a comment

Choose a reason for hiding this comment

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

2489

kalid117658
kalid117658 previously approved these changes Sep 13, 2020
@nikhil550
Copy link
Contributor

The tutorial should eventually contain the asset transfer flow that is contained in the readme here: https://github.com/hyperledger/fabric-samples/tree/master/asset-transfer-private-data/chaincode-go. Eventually we should aim to remove that readme and link to the tutorial. If the tutorial works from e2e, I am ok merging it and iterating from there, so that we are away from the dependency on the old private data smart contract.

I think we can also replace the commands to use the chaincode lifecycle with the command to deploy using the test network script to shorten the tutorial, but that can also follow later.

@denali49
Copy link
Contributor Author

denali49 commented Sep 14, 2020 via email

@denali49
Copy link
Contributor Author

denali49 commented Sep 14, 2020 via email

Copy link
Contributor

@lindluni lindluni left a comment

Choose a reason for hiding this comment

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

Why the addition of the two Pipfile's? Especially the one in the root of the repo?

@denali49
Copy link
Contributor Author

denali49 commented Sep 14, 2020 via email

@denali49
Copy link
Contributor Author

The tutorial should eventually contain the asset transfer flow that is contained in the readme here: https://github.com/hyperledger/fabric-samples/tree/master/asset-transfer-private-data/chaincode-go. Eventually we should aim to remove that readme and link to the tutorial. If the tutorial works from e2e, I am ok merging it and iterating from there, so that we are away from the dependency on the old private data smart contract.

I think we can also replace the commands to use the chaincode lifecycle with the command to deploy using the test network script to shorten the tutorial, but that can also follow later.

I added an asset transfer header, to specifically call this out. The original direction was to integrate the private data README into the existing tutorial which I took to mean that we wanted to keep the flow of the tutorial. We can certainly just adopt the README flow as well and drop the peer chaincode lifecycle piece in favor of the script approach to make it shorter, but I agree we should merge for now and do that in another PR.

@nikhil550
Copy link
Contributor

The tutorial should eventually contain the asset transfer flow that is contained in the readme here: https://github.com/hyperledger/fabric-samples/tree/master/asset-transfer-private-data/chaincode-go. Eventually we should aim to remove that readme and link to the tutorial. If the tutorial works from e2e, I am ok merging it and iterating from there, so that we are away from the dependency on the old private data smart contract.
I think we can also replace the commands to use the chaincode lifecycle with the command to deploy using the test network script to shorten the tutorial, but that can also follow later.

I added an asset transfer header, to specifically call this out. The original direction was to integrate the private data README into the existing tutorial which I took to mean that we wanted to keep the flow of the tutorial. We can certainly just adopt the README flow as well and drop the peer chaincode lifecycle piece in favor of the script approach to make it shorter, but I agree we should merge for now and do that in another PR.

I agree that we need to maintain the current flow of the tutorial, at least in terms of keeping the technical sections. we can work to integrate and improve the flow until the readme is redundant.

docs/source/private_data_tutorial.rst Outdated Show resolved Hide resolved
docs/source/private_data_tutorial.rst Outdated Show resolved Hide resolved
docs/source/private_data_tutorial.rst Outdated Show resolved Hide resolved
docs/source/private_data_tutorial.rst Show resolved Hide resolved
docs/source/private_data_tutorial.rst Outdated Show resolved Hide resolved
docs/source/private_data_tutorial.rst Outdated Show resolved Hide resolved
docs/source/private_data_tutorial.rst Outdated Show resolved Hide resolved
docs/source/private_data_tutorial.rst Outdated Show resolved Hide resolved
docs/source/private_data_tutorial.rst Show resolved Hide resolved
docs/source/private_data_tutorial.rst Outdated Show resolved Hide resolved
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.

Ran fine on my computer. I have some comments on the flow in addition to the existing comments. Ill let you address, but we may want to merge and iterate and some point.

docs/source/private_data_tutorial.rst Show resolved Hide resolved
docs/source/private_data_tutorial.rst Show resolved Hide resolved
Signed-off-by: Chris Gabriel <chris_gabriel_98@yahoo.com>

fix invoke command on first ReadAsset

Signed-off-by: Chris Gabriel <chris_gabriel_98@yahoo.com>

minor grammatical fixes

Signed-off-by: Chris Gabriel <chris_gabriel_98@yahoo.com>

resolve comments

Signed-off-by: Chris Gabriel <chris_gabriel_98@yahoo.com>

Add Asset Transfer section header and delete pipfile

Signed-off-by: Chris Gabriel <chris_gabriel_98@yahoo.com>

address comments and add use case section

Signed-off-by: Chris Gabriel <chris_gabriel_98@yahoo.com>
@denali49
Copy link
Contributor Author

Why the addition of the two Pipfile's? Especially the one in the root of the repo?

@btl5037 this has been addressed for some time, please review so we can get merged.

@mergify mergify bot merged commit 17858a5 into hyperledger:master Sep 20, 2020
@denali49
Copy link
Contributor Author

@Mergifyio backport release-2.2

@mergify
Copy link

mergify bot commented Sep 21, 2020

Command backport release-2.2: failure

No backport have been created

  • Backport to branch release-2.2 failed

Cherry-pick of 17858a5 has failed:

On branch mergify/bp/release-2.2/pr-1875
Your branch is up to date with 'origin/release-2.2'.

You are currently cherry-picking commit 17858a570.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:

	modified:   docs/source/images/SideDB-org1-org2.png
	modified:   docs/source/images/SideDBTutorialImages.pptx

Unmerged paths:
  (use "git add <file>..." to mark resolution)

	both modified:   docs/source/private_data_tutorial.rst

@denali49 denali49 deleted the pvtdata-tutorial branch June 2, 2021 14:03
WaleedMortaja pushed a commit to WaleedMortaja/fabric that referenced this pull request Jun 1, 2022
…1875)

(cherry picked from commit 17858a5)

Signed-off-by: Waleed Mortaja <waleedmortaja@protonmail.com>

Update 'Using Private Data in Fabric' tutorial

Signed-off-by: Chris Gabriel <chris_gabriel_98@yahoo.com>

fix invoke command on first ReadAsset

Signed-off-by: Chris Gabriel <chris_gabriel_98@yahoo.com>

minor grammatical fixes

Signed-off-by: Chris Gabriel <chris_gabriel_98@yahoo.com>

resolve comments

Signed-off-by: Chris Gabriel <chris_gabriel_98@yahoo.com>

Add Asset Transfer section header and delete pipfile

Signed-off-by: Chris Gabriel <chris_gabriel_98@yahoo.com>

address comments and add use case section

Signed-off-by: Chris Gabriel <chris_gabriel_98@yahoo.com>
denyeart pushed a commit that referenced this pull request Jun 1, 2022
(cherry picked from commit 17858a5)

Signed-off-by: Waleed Mortaja <waleedmortaja@protonmail.com>

Update 'Using Private Data in Fabric' tutorial

Signed-off-by: Chris Gabriel <chris_gabriel_98@yahoo.com>

fix invoke command on first ReadAsset

Signed-off-by: Chris Gabriel <chris_gabriel_98@yahoo.com>

minor grammatical fixes

Signed-off-by: Chris Gabriel <chris_gabriel_98@yahoo.com>

resolve comments

Signed-off-by: Chris Gabriel <chris_gabriel_98@yahoo.com>

Add Asset Transfer section header and delete pipfile

Signed-off-by: Chris Gabriel <chris_gabriel_98@yahoo.com>

address comments and add use case section

Signed-off-by: Chris Gabriel <chris_gabriel_98@yahoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants