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

Implement 'osnadmin channel info' subcommand and update tests #4890 #4919

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Priyansurout
Copy link
Contributor

@Priyansurout Priyansurout commented Jul 4, 2024

Implemented the osnadmin channel info subcommand to retrieve detailed information about a specific channel from an Ordering Service Node (OSN). This involved modifying the CLI commands and adding corresponding functionality to interact with the channel participation API.

Type of change
New feature
Implemented the osnadmin channel info subcommand to retrieve detailed information about a specific channel from an Ordering Service Node (OSN).

Introduced info subcommand under osnadmin channel to support querying individual channel details.
Updated existing tests and added new tests to cover the new feature and edge cases.

@Priyansurout Priyansurout requested review from a team as code owners July 4, 2024 18:13
@Priyansurout Priyansurout changed the title Implement 'osnadmin channel info' subcommand and update tests #4892 Implement 'osnadmin channel info' subcommand and update tests #4890 Jul 4, 2024
Copy link
Contributor

@satota2 satota2 left a comment

Choose a reason for hiding this comment

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

@Priyansurout Thank you for the updates.

I found some parts that needed correction, so I have added some suggestion comments in the relevant places. Please check them.

Additionally, I could not find the updates to the test code mentioned in the PR title. This PR might still be in draft status, and the updates may be added later, but I wanted to bring this to your attention just in case.

docs/wrappers/osnadmin_channel_postscript.md Outdated Show resolved Hide resolved

Here are some examples of the `osnadmin channel info` command.

- Using the `--channel-id` flag to get detailed information for mychannel from the orderer at `orderer.example.com:9443`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Using the `--channel-id` flag to get detailed information for mychannel from the orderer at `orderer.example.com:9443`.
- Using the `--channelID` flag to get detailed information for mychannel from the orderer at `orderer.example.com:9443`.

docs/wrappers/osnadmin_channel_postscript.md Outdated Show resolved Hide resolved
Comment on lines +61 to +65
"name": "mychannel",
"url": "/participation/v1/channels/mychannel",
"consensusRelation": "consenter",
"status": "active",
"height": 3
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"name": "mychannel",
"url": "/participation/v1/channels/mychannel",
"consensusRelation": "consenter",
"status": "active",
"height": 3
"name": "mychannel",
"url": "/participation/v1/channels/mychannel",
"consensusRelation": "consenter",
"status": "active",
"height": 3

Priyansurout and others added 2 commits July 8, 2024 07:43
Co-authored-by: Tatsuya Sato <tatsuya.sato.so@hitachi.com>
Signed-off-by: Priyansu rout <65958054+Priyansurout@users.noreply.github.com>
Co-authored-by: Tatsuya Sato <tatsuya.sato.so@hitachi.com>
Signed-off-by: Priyansu rout <65958054+Priyansurout@users.noreply.github.com>
@denyeart
Copy link
Contributor

denyeart commented Jul 8, 2024

All of the commits must be signed to pass DCO check. Better yet, a pull request with a single feature should ideally be delivered as a single commit. See the instructions on how to squash multiple commits into a single commit for this pull request, and how to amend commits in future pull requests - https://hyperledger-fabric.readthedocs.io/en/latest/github/github.html#updating-a-pull-request

@tock-ibm
Copy link
Contributor

tock-ibm commented Aug 1, 2024

What is the motivation for this change? all the info about a channel can be retrieved with:

GET <...>/participation/v1/channels/channelID

All that is added is another method in the cli:

osnadmincli info -c channelID

which is completely equivalent to the existing:

osnadmincli list -c channelID

as they both emit the same GET request.

So what is the point? Am I missing something here?

@@ -114,10 +117,12 @@ func executeForArgs(args []string) (output string, exit int, err error) {
resp, err = osnadmin.Join(osnURL, marshaledConfigBlock, caCertPool, tlsClientCert)
case list.FullCommand():
if *listChannelID != "" {
resp, err = osnadmin.ListSingleChannel(osnURL, *listChannelID, caCertPool, tlsClientCert)
resp, err = osnadmin.Info(osnURL, *listChannelID, caCertPool, tlsClientCert)
Copy link
Contributor

Choose a reason for hiding this comment

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

So now we have two ways of getting the same information about a channel. Why do we need that?
Renaming the existing API is not a good idea as it might break existing tools that use the current CLI.

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