Fix client command detection when flags precede subcommand#506
Conversation
bestbeforetoday
left a comment
There was a problem hiding this comment.
Thank you very much for this contribution. Just a concern with one of the unit tests (see inline comments); otherwise looks good to me.
| defYaml := util.GetDefaultConfigFile("fabric-ca-client") | ||
| os.Remove(defYaml) | ||
| defer os.Remove(defYaml) | ||
|
|
||
| err := RunMain([]string{cmdName, "--loglevel", "warning", "enroll", "-d"}) | ||
| if assert.Error(t, err) { | ||
| assert.NotContains(t, err.Error(), "Enrollment information does not exist", | ||
| "enroll with flags before subcommand should not require prior enrollment") | ||
| } |
There was a problem hiding this comment.
I think this configuration should be guaranteed to fail so it should require an error, not just assert if an error occurs. The NotContians assertion on the error message also seems error-prone since the error messages could be changed in the future, which would make this test pass regardless of actual behaviour.
I think this really needs to change to a positive test:
| defYaml := util.GetDefaultConfigFile("fabric-ca-client") | |
| os.Remove(defYaml) | |
| defer os.Remove(defYaml) | |
| err := RunMain([]string{cmdName, "--loglevel", "warning", "enroll", "-d"}) | |
| if assert.Error(t, err) { | |
| assert.NotContains(t, err.Error(), "Enrollment information does not exist", | |
| "enroll with flags before subcommand should not require prior enrollment") | |
| } | |
| adminHome := filepath.Join(tdDir, "enrolladminhome") | |
| // Remove admin home directory if it exists | |
| err := os.RemoveAll(adminHome) | |
| require.NoErrorf(t, err, "Failed to remove directory %s: %s", adminHome, err) | |
| // Remove admin home directory that this test is going to create before | |
| // exiting the test case | |
| defer os.RemoveAll(adminHome) | |
| srv := setupEnrollTest(t) | |
| // Cleanup before exiting the test case | |
| defer stopAndCleanupServer(t, srv) | |
| err = RunMain([]string{cmdName, "--loglevel", "warning", "enroll", "-d", "-u", enrollURL, "-H", adminHome}) | |
| require.NoError(t, err, "enroll should succeed with global flags before subcommand") |
There was a problem hiding this comment.
Good point — updated to a positive test that enrolls against a running CA server with global flags before the subcommand. Thanks for the suggestion.
…er#465) Use cobra's Find to resolve the top-level subcommand instead of assuming args[1] is always the command name. This fixes incorrect enrollment validation when global flags such as --loglevel appear before enroll, gencsr, getcacert, or getcainfo. Signed-off-by: antcybersec <anant1234466@gmail.com>
22a8da8 to
47f15ac
Compare
Summary
Findinstead of assumingargs[1]is always the command name--loglevel warning) appear beforeenroll,gencsr,getcacert, orgetcainfoProblem
fabric-ca-client --loglevel warning enroll -u ...was treated as if the subcommand waswarning, soConfigInitincorrectly required prior enrollment and failed with:Test plan
go test ./cmd/fabric-ca-client/command/ -run 'TestResolveCommandName|TestEnrollWithGlobalFlagsBeforeSubcommand'go test ./cmd/fabric-ca-client/command/Fixes #465