feat!: update cli commands #211

Chief-Rishab · 2023-02-22T10:17:51Z

Add/Update CLI commands

BREAKING CHANGES: serve and migrate command changed to server start, server migrate,
configs changed to config init and config list

coveralls · 2023-02-22T10:21:16Z

Pull Request Test Coverage Report for Build 4363037925

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.5%) to 85.635%

Totals
Change from base Build 4321301974:	-0.5%
Covered Lines:	4644
Relevant Lines:	5423

💛 - Coveralls

ravisuhag · 2023-02-22T11:26:59Z

@sudo-suhas @StewartJingga can you take a look at this. If looks good we can merge this. This follows similar guidelines as we are using in Guardian for CLI.

sudo-suhas · 2023-02-22T12:13:03Z

@ravisuhag I am on leave till 24th, will check once I am back.

sudo-suhas · 2023-02-24T05:37:40Z

Makefile

+
+clean-doc:
+	@echo "> cleaning up auto-generated docs"
+	@rm -rf ./docs/docs/reference/cli


Suggested change

@rm -rf ./docs/docs/reference/cli

@rm -rf ./docs/docs/reference/cli.md

sudo-suhas · 2023-02-24T05:57:02Z

cli/assets.go

 		Use:   "view <id>",
 		Short: "view asset for the given ID",
 		Example: heredoc.Doc(`
 			$ compass asset view <id>


Can we update supported commands to use urn instead of ID?

sudo-suhas · 2023-02-24T05:58:16Z

cli/assets.go

 }
+
+func listAllTypesCommand() *cobra.Command {
+	var types, services, data, q, q_fields string


Lets follow Go's naming convention and rename q_fields to qFields or just fields (context conveys that it is for the query)

Can we keep the type for data as map[string]string itself? We can use cmd.Flags().StringToStringVarP(...) to handle the unmarshaling.

One obvious improvement would be to directly use an instance of compassv1beta1.GetAllTypesRequest and pass the pointers to it's field so that there is no additional step required for setting these fields and create a new request.

sudo-suhas · 2023-02-24T06:03:46Z

cli/assets.go

+
+func makeGetAllTypesRequest(types, services, data, q, q_fields string) *compassv1beta1.GetAllTypesRequest {
+	newReq := &compassv1beta1.GetAllTypesRequest{}
+	if types != "" {


Is there any value in having this if check? compassv1beta1.GetAllTypesRequest.Types is a string field so setting it to "" is not a problem. Similarly for other fields being set conditionally, is it required?

Was already there in the current CLI code, will change if it's required

Lets refactor where it makes sense.

sudo-suhas · 2023-02-24T06:04:56Z

cli/assets.go

+			report := [][]string{}
+			report = append(report, []string{"NAME", "COUNT"})


Suggested change

report := [][]string{}

report = append(report, []string{"NAME", "COUNT"})

report := [][]string{{"NAME", "COUNT"}}

sudo-suhas · 2023-02-24T07:10:56Z

cli/server.go

-		Short:   "Serve gRPC & HTTP service",
-		Long:    heredoc.Doc(`Serve gRPC & HTTP on a port defined in PORT env var.`),
-		Aliases: []string{"server", "start"},
+func serverCmd() *cobra.Command {


Currently not able to test these since the config loading is broken.

can you once again try with --config flag to use the file from current directory. Alternatively you can use the config file created after init command.

will test once the current set of review comments are addressed.

not sure if you realised but #211 (comment) means that loading config is also broken.

sudo-suhas · 2023-02-24T07:12:50Z

cli/server.go

 	}

-	esClient, err := initElasticsearch(logger, config.Elasticsearch)
+	esClient, err := initElasticsearch(logger)


Pass in the configs explicitly.

sudo-suhas · 2023-02-24T07:13:12Z

cli/server.go

 }

-func runServer(config Config) error {
+func runServer() error {


Undo this change please.

sudo-suhas · 2023-02-24T07:17:23Z

cli/version.go

+		Short:   "Print version information",
+		RunE: func(cmd *cobra.Command, args []string) error {
+			if Version == "" {
+				fmt.Println(term.Yellow("Version history not available"))


Any reason we are printing 'Version history'? Shouldn't it just be 'Version'?

sudo-suhas · 2023-02-24T07:20:17Z

main.go

+
+	rootCmd := cli.New(cliConfig)
+
+	if cmd, err := rootCmd.ExecuteC(); err != nil {


Can we run with ExecuteContextC instead? We can pass in a context created with signal.NotifyContext.

sudo-suhas · 2023-02-24T07:33:35Z

main.go

+		flagErr := strings.HasPrefix(err.Error(), "unknown flag")
+		sflagErr := strings.HasPrefix(err.Error(), "unknown shorthand flag")
+
+		if cmdErr || flagErr || sflagErr {


Can you double-check the handling for this? Usage is being printed twice currently:

$ ./compass config --test Usage: compass config <command> [flags] Available commands: init list unknown flag: --test Usage: compass config <command> [flags] Available commands: init list

Chief-Rishab · 2023-03-01T06:50:42Z

cli/config.go

 		opts = append(opts,
 			config.WithPath("./"),
-			config.WithName("compass"),
+			config.WithName("compass.yaml"),


@sudo-suhas laoding the options WithName("compass") gives an error error in reading config: yaml when the conifg file compass.yaml is missing in the root folder.

Instead it should be giving an error, file not found. Currently it is trying to use the compass executable while trying to read the configs when the file is missing.

I also tried to add the WithType("yaml") but it still gives the same error. Can we rename the compass.yaml to something like .conifg.yaml.

I need the correct error to be handled for searching for file made from the init command, which will be called only when the config file is missing in the root.

The extention "yaml" in line 118 will be discarded once we go ahead with something which throws the right error.

I am not clear on why we care about the exact error message being returned. We were anyway proceeding with defaults when the config file was not found.

kushsharma · 2023-03-06T11:44:20Z

cli/config.go

+		Example: heredoc.Doc(`
+			$ compass config init
+		`),
+		// SilencePersistentFlag: true,


clean this up?

kushsharma · 2023-03-06T11:47:16Z

cli/config.go

+			_ = yaml.NewEncoder(os.Stdout).Encode(*cfg)
+			return nil


return yaml.NewEncoder(os.Stdout).Encode(*cfg) ?

kushsharma · 2023-03-06T11:49:49Z

cli/config.go

+func LoadConfig() (*Config, error) {
+	var cfg Config
+	err := cmdx.SetConfig("compass").Load(&cfg)
+	if err != nil {
+		if errors.As(err, &config.ConfigFileNotFoundError{}) {
+			return LoadFromCurrentDir()
+		}
+		return &cfg, err
 	}
+	return &cfg, nil
+}

+func LoadFromCurrentDir() (*Config, error) {
 	var cfg Config
+	var opts []config.LoaderOption
+
+	opts = append(opts,
+		config.WithPath("./"),
+		config.WithName("compass.yaml"),
+		config.WithEnvKeyReplacer(".", "_"),
+		config.WithEnvPrefix("COMPASS"),
+	)
+
 	if err := config.NewLoader(opts...).Load(&cfg); err != nil {
 		if errors.As(err, &config.ConfigFileNotFoundError{}) {
-			return cfg, nil
+			return &cfg, ErrConfigNotFound
 		}
-		return cfg, err
+		return &cfg, err
 	}
-	return cfg, nil
+	return &cfg, nil
+}


The current implementation looks like we give lower priority to the config file in the current directory, I feel we should do the opposite, and try to load from the current first if fails, we can fall back to the rest of the pre-defined directories.

kushsharma · 2023-03-06T11:52:56Z

cli/server.go

+}
+
+func serverStartCommand(cfg *Config) *cobra.Command {
+


nit: I see couple of unnecessary new lines, for example in serverMigrateCommand, func New(cliConfig *Config) *cobra.Comman, and here

kushsharma · 2023-03-09T14:33:42Z

LGTM

ravisuhag · 2023-03-09T14:38:26Z

@Chief-Rishab Can you update the docs PR according to the change made here.

Chief-Rishab and others added 20 commits February 6, 2023 15:52

docs: add configuration page, update intro

40be1c7

docs: add cli commands in tour section

721d15f

docs: add intro page to tour

7fdca2d

docs: add tabs in guides

37b0c7d

docs: update guides with cli commands

0553628

docs: upate installation and configuration

e61f194

fix: cli

34cf0a0

docs: add cli reference, make doc, update api

4a84856

cli: add version command

6126b3c

cli: add shell completion command

ed124f1

cli: add environment command

c60372a

refactor: make server cmd to start and migrate

e1362b8

feat(cli): add asset commands

ee4bdb2

docs: update cli commands

389f56b

refactor: cli config cmd, add config init

ecf3202

chore: add changes from suggestions

e29f598

fix: start and migrate server with config flag

b2280e9

chore: add changes from suggestions

488270f

revert: docs

e2c3834

revert: docs

458b804

Chief-Rishab marked this pull request as ready for review February 22, 2023 10:19

Chief-Rishab requested a review from ravisuhag February 22, 2023 10:20

Chief-Rishab requested review from kushsharma, mabdh and sudo-suhas February 22, 2023 10:22

ravisuhag approved these changes Feb 22, 2023

View reviewed changes

ravisuhag requested a review from StewartJingga February 22, 2023 11:26

sudo-suhas requested changes Feb 24, 2023

View reviewed changes

sudo-suhas reviewed Feb 24, 2023

View reviewed changes

Chief-Rishab added 3 commits February 27, 2023 15:54

chore: add changes from suggestion

1c7dc7b

chore: add changes from suggestions

6134b51

chore: add changes from suggestion

3045394

Chief-Rishab commented Mar 1, 2023

View reviewed changes

fix: load config, add persistant flag overloading

49f7ea6

kushsharma requested changes Mar 6, 2023

View reviewed changes

Chief-Rishab added 3 commits March 8, 2023 14:09

chore: load config from current directory first

b157f94

Merge remote-tracking branch 'origin/main' into update-cli

044a8e1

chore: get asset with urn instead of id

d461ded

Chief-Rishab requested review from kushsharma and sudo-suhas March 8, 2023 10:51

ravisuhag merged commit 4524891 into main Mar 9, 2023

ravisuhag deleted the update-cli branch March 9, 2023 14:37

	@rm -rf ./docs/docs/reference/cli
	@rm -rf ./docs/docs/reference/cli.md

		report := [][]string{}
		report = append(report, []string{"NAME", "COUNT"})

	report := [][]string{}
	report = append(report, []string{"NAME", "COUNT"})
	report := [][]string{{"NAME", "COUNT"}}


		rootCmd := cli.New(cliConfig)

		if cmd, err := rootCmd.ExecuteC(); err != nil {

feat!: update cli commands #211

feat!: update cli commands #211

Uh oh!

Conversation

Chief-Rishab commented Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 4363037925

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

ravisuhag commented Feb 22, 2023

Uh oh!

sudo-suhas commented Feb 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chief-Rishab Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kushsharma commented Mar 9, 2023

Uh oh!

ravisuhag commented Mar 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Chief-Rishab commented Feb 22, 2023 •

edited

Loading

coveralls commented Feb 22, 2023 •

edited

Loading

Chief-Rishab Mar 1, 2023 •

edited

Loading