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

osnadmin CLI command reference #2077

Merged
merged 1 commit into from
Nov 9, 2020
Merged

Conversation

wlahti
Copy link
Contributor

@wlahti wlahti commented Nov 5, 2020

Type of change

  • Documentation update

Description

Command reference documentation for the osnadmin CLI.

Related issues

FAB-18309

@wlahti wlahti requested review from a team as code owners November 5, 2020 19:11
Copy link
Contributor

@pamandrejko pamandrejko left a comment

Choose a reason for hiding this comment

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

@wlahti There's something missing here because the command is not showing up in the rendered content:
https://hyperledger-fabric--2077.org.readthedocs.build/en/2077/command_ref.html

Copy link
Contributor

@pamandrejko pamandrejko left a comment

Choose a reason for hiding this comment

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

A few questions

Comment on lines 72 to 80
Path to the file containing the config block
```
Copy link
Contributor

Choose a reason for hiding this comment

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

To be more specific, should this say Path to the file containing the channel genesis block or the latest config block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Comment on lines 3 to 5
The `osnadmin channel` command allows administrators to perform channel-related
operations on an orderer, such as joining a channel, listing the channels an
orderer has joined, and removing a channel.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be appropriate to mention here that the join and remove commands cannot be used if the ordering node is part of a system channel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can certainly add that if you think it'd be beneficial.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Comment on lines 24 to 31
-o, --orderer-address=ORDERER-ADDRESS
Endpoint of the OSN
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure what ORDERER-ADDRESS is meant to refer to. Should this be --orderer-address=ORDERER-ADMIN-LISTENADDRESS?
and instead of Endpoint of the OSN, it should Admin endpoint of the OSN?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can clarify that it's the Admin endpoint of the OSN but the ORDERER-ADDRESS part is just how the CLI library we're using prints that info (I can't change it unless we also change the name of the flag).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Comment on lines +90 to +102
--client-cert=CLIENT-CERT Path to file containing PEM-encoded X509 public
key to use for mutual TLS communication with
the OSN
--client-key=CLIENT-KEY Path to file containing PEM-encoded private key
to use for mutual TLS communication with the
OSN
Copy link
Contributor

Choose a reason for hiding this comment

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

How do they know that these 2 fields are required and not optional throughout these commands?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The flags are all set as required in the implementation (the only optional flag for any the commands is --channel-id on the list command). The CLI will return an error if any required flags are not set. Do we do anything to differentiate the required flags for other commands that you're aware of?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK makes sense. Then I would suggest we update the list command so they know that --channel-id is optional -- if not specified they will get back the details of all the channels the osn is a member of instead of a specific one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have that included in the description for list: "If the channel-id flag is set, more detailed information will be provided for that channel." and also mention it in the example usage for the command.

Do you have specific places you'd like to see more added?

Copy link
Contributor

Choose a reason for hiding this comment

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

For the --ca-file flag, how would they know this is the root cert of the TLS CA and not the organization CA?
Should it be:
Path to file containing PEM-encoded trusted TLS certificate(s) for the OSN
or more specifically:
Path to file containing PEM-encoded trusted root certificate(s) of the TLS CA for the OSN

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reused the flag description that we've been using for years on the peer channel flag of the same name. I can certainly update this if we think it's beneficial.

Copy link
Contributor Author

@wlahti wlahti Nov 9, 2020

Choose a reason for hiding this comment

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

Updated to Path to file containing PEM-encoded TLS CA certificate(s) for the OSN

@wlahti
Copy link
Contributor Author

wlahti commented Nov 5, 2020

@wlahti There's something missing here because the command is not showing up in the rendered content:
https://hyperledger-fabric--2077.org.readthedocs.build/en/2077/command_ref.html

Added it to the command_ref.

Copy link
Contributor

@jyellick jyellick left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me

@@ -50,7 +50,7 @@ func executeForArgs(args []string) (output string, exit int, err error) {

join := channel.Command("join", "Join an Ordering Service Node (OSN) to a channel. If the channel does not yet exist, it will be created.")
joinChannelID := join.Flag("channel-id", "Channel ID").Short('c').Required().String()
configBlockPath := join.Flag("config-block", "Path to the file containing the config block").Short('b').Required().String()
configBlockPath := join.Flag("config-block", "Path to the file containing the config block. This can be either the genesis block or a config block for the channel.").Short('b').Required().String()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this to address another comment? The genesis block is a config block, it's just the first one. Really, this should be the latest config block, or, a config block which contains the current set consenters and corresponding TLS CAs (as required to replicate the channel).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how much we can/should bake into a one/two-liner. Maybe switch back towards the original text with a minor clarification: "Path to the file containing a config block for the channel"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps something like: "Path to the file containing an up to date config block for the channel."? Maybe that creates more confusion than clarity. My biggest concern was that adding the longer statement about genesis or config was verbose without adding info.

Copy link
Contributor Author

@wlahti wlahti Nov 9, 2020

Choose a reason for hiding this comment

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

Done. Switched to "up-to-date config block".

@wlahti wlahti force-pushed the fab-18309 branch 4 times, most recently from 3fb6e0a to 93d718b Compare November 9, 2020 18:03
jyellick
jyellick previously approved these changes Nov 9, 2020
FAB-18309 #done

Signed-off-by: Will Lahti <wtlahti@us.ibm.com>
@wlahti
Copy link
Contributor Author

wlahti commented Nov 9, 2020

Rebased and updated the reference doc for the rename of clusterRelation to consensusRelation.

@jyellick jyellick merged commit 1e90c88 into hyperledger:master Nov 9, 2020
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.

4 participants