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

Conf cli #170

Closed
wants to merge 6 commits into from
Closed

Conf cli #170

wants to merge 6 commits into from

Conversation

MayRosenbaum
Copy link
Contributor

@MayRosenbaum MayRosenbaum commented Jun 12, 2023

work in progress, last commit includes get config command + version command + tests and a README.md file

liran-funaro and others added 2 commits May 17, 2023 10:47
Signed-off-by: Liran Funaro <[email protected]>
cli/main.go Outdated
Comment on lines 7 to 20
"fmt"
"github.com/hyperledger-labs/orion-server/pkg/types"
"io/ioutil"
"os"
"path"
"runtime/debug"

"github.com/hyperledger-labs/orion-sdk-go/pkg/bcdb"
"github.com/hyperledger-labs/orion-sdk-go/pkg/config"
orionconfig "github.com/hyperledger-labs/orion-server/config"
"github.com/hyperledger-labs/orion-server/pkg/logger"
"github.com/pkg/errors"
"github.com/spf13/cobra"
"github.com/spf13/viper"
Copy link
Contributor

Choose a reason for hiding this comment

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

remove empty lines and do goimports

cli/main.go Outdated

func OrionCliInit() *cobra.Command {
cmd := &cobra.Command{
Use: "Orion-CLI",
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned above, it is called bcdbadmin

cli/main.go Outdated
// CAs command
func CAsCmd() *cobra.Command {
cmd := &cobra.Command{
Use: "CAs",
Copy link
Contributor

Choose a reason for hiding this comment

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

cas - lower case commands

cli/main.go Outdated
Short: "manage CA's",
Args: nil,
RunE: func(cmd *cobra.Command, args []string) error {
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe return errors.New("not implemented yet") in all places that are not implemented.

cli/main.go Outdated
Comment on lines 37 to 41
cmd.AddCommand(versionCmd())
cmd.AddCommand(configCmd())
cmd.AddCommand(adminCmd())
cmd.AddCommand(nodeCmd())
cmd.AddCommand(CAsCmd())
Copy link
Contributor

Choose a reason for hiding this comment

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

Basic structure is ok.

cli/main.go Outdated
}

// versionCmd - check what command it is
func versionCmd() *cobra.Command {
Copy link
Contributor

Choose a reason for hiding this comment

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

version is ok

cli/main.go Outdated
}

// config command
func configCmd() *cobra.Command {
Copy link
Contributor

Choose a reason for hiding this comment

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

Move the implementation of each command into a file of its own, and test it.
Lets start with
PR1 - version + basic structure, help etc
PR2 - a full implementation of dcdbadmin config get ..." with tests. PR3 - a full implementation of dcdbadmin config set ..." with tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

For testing, you can use regular unit tests on the implementation functions, using the approach from examples/cars.
For testing the parsing of flags etc you can use the approach from here

cli/main.go Outdated
Comment on lines 109 to 110
RunE: func(cmd *cobra.Command, args []string) error {
err := params.CreateDbAndOpenSession()
Copy link
Contributor

Choose a reason for hiding this comment

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

The implementation of the command should be in a function that receives only what it needs, and is tested separately. see the car example.

cli/README.md Outdated
@@ -0,0 +1,44 @@
# Config Orion via CLI

This command-line tool provides you a simple way to config an orion database server.
Copy link
Contributor

Choose a reason for hiding this comment

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

provides you a -> provides a


This command-line tool provides you a simple way to config an orion database server.

## Commands
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a section about building the tool.

Comment on lines 3 to 18
import (
"encoding/pem"
"github.com/hyperledger-labs/orion-sdk-go/pkg/bcdb"
"github.com/hyperledger-labs/orion-sdk-go/pkg/config"
orionconfig "github.com/hyperledger-labs/orion-server/config"
"github.com/hyperledger-labs/orion-server/pkg/logger"
"github.com/hyperledger-labs/orion-server/pkg/types"
"github.com/pkg/errors"
"github.com/spf13/cobra"
"github.com/spf13/viper"
"gopkg.in/yaml.v2"
"io/ioutil"
"os"
"path"
"strconv"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

goimports

cli/main.go Outdated
Comment on lines 7 to 8
"github.com/hyperledger-labs/orion-sdk-go/cli/commands"
"os"
Copy link
Contributor

Choose a reason for hiding this comment

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

go imports

cli/README.md Outdated
##### Flags
| Flags | Description |
|-----------------------------|-------------------------------------------------------------------|
| `-c, --cli-config-path` | the absolute path of CLI connection configuration file |
Copy link
Contributor

Choose a reason for hiding this comment

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

a relative path works too

"github.com/spf13/cobra"
)

func NodeCmd() *cobra.Command {
Copy link
Contributor

Choose a reason for hiding this comment

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

it is not in the root

Comment on lines 17 to 18
cmd.AddCommand(VersionCmd())
cmd.AddCommand(configCmd())
Copy link
Contributor

Choose a reason for hiding this comment

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

what about all the rest? either we add it here, or we don't have them at all in this PR. I am ok with adding them and having them return "not implemented"

Copy link
Contributor

Choose a reason for hiding this comment

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

In addition, test this like you test the version, make sure it responds to help config etc, but not to command "fake", i.e. testing the usage

require.NoError(t, err)
err = compareFiles("../testdata/crypto/server/server.pem", "../../configTestRes/nodes/orion-server1.pem")
require.NoError(t, err)
err = compareFiles("../testdata/crypto/CA/CA.pem", "../../configTestRes/rootCAs/rootCA0.pem")
Copy link
Contributor

Choose a reason for hiding this comment

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

under testdata add a folder expected-get, and in it put the files you compare against.
this folder should include the a file version.yaml that includes that types.Version you read.


func TestVersionCommand(t *testing.T) {
// 1. Create BCDBHTTPServer and start server
testServer, err := SetupTestServer(t)
Copy link
Contributor

Choose a reason for hiding this comment

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

as above...
it leaves artifacts in the repo:

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	cli/commands/ledger/
	cli/commands/tmp/

you need to change the server config to some tmp location and cleanup afterwards, as done in examples

Comment on lines +18 to +19
err = rootCmd.Execute()
require.NoError(t, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not enough to test that there is no error, you can capture the output and check you get the desired output:

b := &strings.Builder{}
	rootCmd.SetOut(b)
	err = rootCmd.Execute()
	require.NoError(t, err)
	fmt.Printf(">>> %s \n", b.String()) // compare to expected

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.

3 participants