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

Deprecated fabric tools image #4775

Merged
merged 1 commit into from
May 1, 2024

Conversation

SamYuan1990
Copy link
Contributor

@SamYuan1990 SamYuan1990 commented Mar 30, 2024

Type of change

Deprecated fabric tools image

Description

Deprecated fabric tools image

Additional details

Remove tools image related code for github action, make file, docker file, test data, test case.

Related issues

hyperledger/fabric-samples#1186

@SamYuan1990
Copy link
Contributor Author

open this PR as ready for review, for merge, please wait for hyperledger/fabric-samples#1193

@SamYuan1990 SamYuan1990 marked this pull request as ready for review April 6, 2024 11:48
@SamYuan1990 SamYuan1990 requested a review from a team as a code owner April 6, 2024 11:48
@denyeart
Copy link
Contributor

Tests are passing now, but I'm still waiting for hyperledger/fabric-samples#1193 to finalize before merging this one.

@denyeart
Copy link
Contributor

denyeart commented Apr 18, 2024

I think the 3 files in /integration/lifecycle/testdata can remain.
The test /integration/lifecycle/ccenv14_test.go is a compatibility test and says that fabric-tools from v1.4 was used. That older fabric-tools image will remain forever. So getting rid of fabric-tools in main branch (for v3.0) should have no impact.

Or did I misunderstand something?

These files will probably be removed soon during the v1 lifecycle removal.

@denyeart
Copy link
Contributor

I found 3 more references to fabric-tools that should be removed:

docs/source/test_network.md:117:1667543b5634   hyperledger/fabric-tools:latest     "/bin/bash"         1 second ago    Up Less than a second                                                    cli
docs/source/create_channel/create_channel_test_net.md:52:1667543b5634   hyperledger/fabric-tools:latest     "/bin/bash"         1 second ago    Up Less than a second                                                    cli
docs/source/channel_update_tutorial.rst:177:.. note:: the `./addOrg3.sh up` command uses a `fabric-tools` CLI container to perform 

The first two are docker outputs, simply delete the fabric-tools line.
The last one is a doc note which can be removed.

@denyeart
Copy link
Contributor

Also, the prereqs topic should be updated to indicate that jq is required for any tutorials based on test-network sample, right?
It currently says you only need it for channel config tutorial.

@SamYuan1990
Copy link
Contributor Author

SamYuan1990 commented Apr 19, 2024

@denyeart , please allow me double confirm with your review comments, to make sure we are on the same page.

  • for the test files, we can keep it remain, as those should be removed with chaincode lifecycle but not fabric tools image?
  • for document, as there is a document repo as https://github.com/hyperledger/fabric-docs-i18n should we update the document repo but not this repo?

@denyeart
Copy link
Contributor

@denyeart , please allow me double confirm with your review comments, to make sure we are on the same page.

  • for the test files, we can keep it remain, as those should be removed with chaincode lifecycle but not fabric tools image?

Yes, let's keep the tests that use the older 1.4 image for now.

For documentation, we always keep the docs in fabric repository /docs directory in sync with the fabric repository code. Ideally a PR that updates code should also update the corresponding doc topics.
https://github.com/hyperledger/fabric-docs-i18n always lag, they can be updated later by the translators.

@SamYuan1990
Copy link
Contributor Author

Also, the prereqs topic should be updated to indicate that jq is required for any tutorials based on test-network sample, right? It currently says you only need it for channel config tutorial.

do we need to update on jq? https://github.com/hyperledger/fabric-samples/pull/1186/files#diff-dc66fa3a567bc48e07f4e161a3c32dba4b27796803decc96e06c24d4ac9278eaR41
https://github.com/hyperledger/fabric-samples/pull/1193/files#diff-dc66fa3a567bc48e07f4e161a3c32dba4b27796803decc96e06c24d4ac9278eaR41

from the code logic, it seems the description for jq is correct as to handle with channel configuration tx.

(only required for the tutorials related to channel configuration transactions).

or should we make it as:

### JQ
Install the latest version of [jq](https://stedolan.github.io/jq/download/) if it is not already installed, it's required for the tutorials related to channel configuration transactions in most of the tutorials from fabric sample to handle json format.

@SamYuan1990 SamYuan1990 requested a review from a team as a code owner April 20, 2024 09:04
@denyeart
Copy link
Contributor

@SamYuan1990
Sorry for delay, I had to test it myself... simply starting test-network requires jq installed locally because when creating the channel, the config update for anchor peers requires it at:
https://github.com/hyperledger/fabric-samples/blob/main/test-network/scripts/createChannel.sh#L127-L131

Therefore the prereq doc should state:

Install the latest version of [jq](https://stedolan.github.io/jq/download/) if it is not already installed, it's required for the tutorials that utilize the fabric-samples test-network.

@satota2
I've created one more PR for fabric-samples so that the user gets a good error message if jq is called but not found locally:
hyperledger/fabric-samples#1200

Signed-off-by: Sam Yuan <yy19902439@126.com>
@SamYuan1990
Copy link
Contributor Author

@SamYuan1990 Sorry for delay, I had to test it myself... simply starting test-network requires jq installed locally because when creating the channel, the config update for anchor peers requires it at: https://github.com/hyperledger/fabric-samples/blob/main/test-network/scripts/createChannel.sh#L127-L131

Therefore the prereq doc should state:

Install the latest version of [jq](https://stedolan.github.io/jq/download/) if it is not already installed, it's required for the tutorials that utilize the fabric-samples test-network.

@satota2 I've created one more PR for fabric-samples so that the user gets a good error message if jq is called but not found locally: hyperledger/fabric-samples#1200

thanks, updated.

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.

Thank you!

@denyeart denyeart merged commit bbf090c into hyperledger:main May 1, 2024
13 of 14 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