json manipulation automation skeleton - #197
MayRosenbaum wants to merge 1 commit into
Conversation
Signed-off-by: May.Buzaglo <May.Buzaglo@ibm.com>
|
coverage: 82.102% (+0.1%) from 82.004% — MayRosenbaum:json_manipulation_automation_skeleton into hyperledger:main |
| node := party.Command("node", "Change one party node's endpoint and/or certificates (any subset of fields).") | ||
| partyID := node.Flag(flagParty, "Party ID.").Required().Uint32() | ||
| role := node.Flag(flagRole, "Node role.").Required().Enum("router", "batcher", "consenter", "assembler") | ||
| shard := node.Flag(flagShard, "Batcher shard ID (required for --role batcher).").Uint32() |
There was a problem hiding this comment.
--shard is documented "required for --role batcher" but is not enforced. The flag is optional and unvalidated, so modify party node --party N --role batcher --block B (omitting --shard) parses successfully and passes Shard: 0. Once the handler is implemented it will silently target shard 0 instead of erroring. Enforce that --shard is set when --role batcher (and rejected otherwise).
| func (c *CLI) registerPartyCAOp(ca *kingpin.CmdClause, op string) { | ||
| cmd := ca.Command(op, "Apply the "+op+" operation to a party's CA / TLS-CA certificate lists.") | ||
| partyID := cmd.Flag(flagParty, "Party whose CA list(s) change.").Required().Uint32() | ||
| signCerts := cmd.Flag(flagSignCert, "PEM path(s) for the signing-CA list (repeatable).").ExistingFiles() |
There was a problem hiding this comment.
modify party ca add|remove|set requires neither --sign-cert nor --tls-cert. modify party ca set --party N --block B (no cert flags) parses with two empty lists; when implemented, set would silently clear both CA/TLS-CA lists, and add/remove become silent no-ops. Require at least one of the two lists.
| type NodeChange struct { | ||
| Party uint32 | ||
| Role string | ||
| Shard uint32 |
There was a problem hiding this comment.
Shard/Port as uint32 with "unchanged if omitted" cannot represent an explicit 0. kingpin yields 0 both when --shard/--port is omitted and when --shard 0/--port 0 is passed, so the handler cannot distinguish "leave unchanged" from an explicit value. ShardID 0 is a valid proto value (proto default), so modify party node will be unable to set/keep shard 0 unambiguously. Consider *uint32 or a separate "was-set" signal.
| "github.com/cockroachdb/errors" | ||
| "github.com/hyperledger/fabric-lib-go/common/flogging" | ||
|
|
||
| "github.com/hyperledger/fabric-x-common/tools/fxadmin/core/cli" |
There was a problem hiding this comment.
Layering inversion: the modify core package imports the cli presentation package. PartyNode/PartyCA take cli.NodeChange/cli.CAChange, so this core package depends on cli, unlike every other core handler (tx, follow, ledger, decode, update), which satisfy the CLI-defined interfaces structurally with primitive args and never import cli. This misplaces the domain DTOs in the presentation layer and risks a future import cycle. Define NodeChange/CAChange in modify (or a neutral package), or pass primitives like the other handlers.
| party := modify.Command("party", "Add or remove ARMA parties, or change a party's nodes and CA lists.") | ||
|
|
||
| add := party.Command("add", "Add a new ARMA party and its orderer organization, if it does not already exist.") | ||
| partyDef := add.Flag(flagParty, "Path to the party definition YAML.").Required().ExistingFile() |
There was a problem hiding this comment.
The --party flag has inconsistent meaning across sibling subcommands. Here (modify party add) --party is a YAML file path (.ExistingFile()), but in modify party node and modify party ca (lines 364, 398) the same --party flag is a numeric PartyID (.Uint32()). Likewise --org is a YAML path in modify app add but an org name in modify app known-certs. Overloading one flag name with two semantics is error-prone; use distinct flag names (e.g. --party-def vs --party-id).
|
|
||
| remove := party.Command("remove", | ||
| "Remove an ARMA party (and its orderer org, unless another party is still associated with it).") | ||
| partyID := remove.Arg("party-id", "Numeric PartyID to remove.").Required().Uint32() |
There was a problem hiding this comment.
Inconsistent invocation style for the same identifier across sibling subcommands. party remove takes the PartyID as a positional Arg, while party node/party ca take it as the --party flag; similarly app remove takes the org name as a positional Arg while app known-certs add|remove take --org. Pick one convention (positional or flag) for the target identifier so the command surface is predictable.
| if *pb { | ||
| return c.handlers.Update.RunFromBlocks(*current, *modified, *output) | ||
| } | ||
| if *currentBlock == "" { |
There was a problem hiding this comment.
No negative test covers this new JSON-mode branch. Dropping .Required() from --current-block (it is now only enforced here at run time) is untested: there is no case asserting that compute-update <current> <modified> --output x (no --pb, no --current-block) returns this error. Add a TestParseErrors/routing case so the run-time guard cannot regress silently.
3 participants
Type of change
Description
JSON manipulation automation in fxadmin - skeleton
Related issues
issue #187