Add CLI commands to manage config file #671

theganyo · 2022-07-25T18:33:47Z

Commands added:

registry config [command]

Available Commands:
  configurations Maintain configuration profiles
  get            Print the value of a property.
  list           List properties for the currently active configuration.
  set            Set the value of a property.
  unset          Unset the value of a property.


registry config configurations [command]

Available Commands:
  activate    Activates an existing named configuration.
  create      Creates a new named configuration.
  delete      Deletes a named configuration.
  describe    Describes a named configuration by listing its properties.
  list        Lists existing named configs.

Closes #649

codecov · 2022-07-25T19:28:11Z

Codecov Report

Merging #671 (c5715d3) into main (7935c83) will increase coverage by 1.40%.
The diff coverage is 74.94%.

@@            Coverage Diff             @@
##             main     #671      +/-   ##
==========================================
+ Coverage   54.15%   55.56%   +1.40%     
==========================================
  Files          78       88      +10     
  Lines        7336     7699     +363     
==========================================
+ Hits         3973     4278     +305     
- Misses       2940     2966      +26     
- Partials      423      455      +32

Impacted Files	Coverage Δ
cmd/registry/cmd/config/configurations/create.go	`58.62% <58.62%> (ø)`
cmd/registry/cmd/config/configurations/activate.go	`60.86% <60.86%> (ø)`
pkg/connection/config.go	`67.24% <67.24%> (ø)`
cmd/registry/cmd/config/unset.go	`70.00% <70.00%> (ø)`
cmd/registry/cmd/config/configurations/describe.go	`75.00% <75.00%> (ø)`
cmd/registry/cmd/config/config.go	`81.48% <81.48%> (ø)`
cmd/registry/cmd/config/configurations/list.go	`82.35% <82.35%> (ø)`
cmd/registry/cmd/config/set.go	`82.35% <82.35%> (ø)`
cmd/registry/cmd/config/get.go	`86.95% <86.95%> (ø)`
cmd/registry/cmd/config/list.go	`86.95% <86.95%> (ø)`
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

seaneganx

Lots of questions and some suggestions.

pkg/connection/config.go

seaneganx · 2022-07-26T20:38:18Z

pkg/connection/config.go

+
+// Flags defines Flags that may be bound to a Configuration. Use like:
+// `cmd.PersistentFlags().AddFlagSet(connection.Flags)`
+var Flags *pflag.FlagSet = createFlagSet()


Can we avoid relying on global state here?

Maybe. I'll look into it.

This actually seems a bit tricky... Instead of trying to rework it right now, we could potentially change access to the FlagSet via a public accessor for now instead of the public variable... That would at least give us a point of control for future work. Otherwise, I think this will be a significant refactor that should probably also incorporate the separating the config and client concerns mentioned in the other comment - and maybe in a follow up PR?

seaneganx · 2022-07-26T20:44:35Z

pkg/connection/grpctest/server.go

-	settings, _ := connection.ReadSettings("")
-	if settings.Address != "" && settings.Validate() == nil {
-		log.Printf("Client will use remote registry at: %s", settings.Address)
+	config, _ := connection.ReadConfig("")


If ReadConfig truly never returns an error when the argument is "", I think we should have a different function (without the empty argument and unused return value) for this purpose.

It can return an error, I just don't care in this case. I added a comment.

cmd/registry/cmd/test/test.go

seaneganx · 2022-07-26T21:23:39Z

pkg/connection/client.go

 func NewRegistryClient(ctx context.Context) (RegistryClient, error) {
-	settings, err := ActiveSettings()
+	config, err := ActiveConfig()


Have we considered removing this "on demand" loading of config values? In most (all?) cases we create one client at the start of the program and pass it along to anywhere it's needed. By loading the config once and setting up the client accordingly, we might be able to eliminate quite a bit of code.

I'm unsure where we'd save a lot of code, but I generally like the idea of separating the concerns. We should maybe look at that in another issue, though?

cmd/registry/cmd/config/configurations/list.go

cmd/registry/cmd/config/set.go

pkg/connection/config.go

pkg/connection/config_test.go

timburks · 2022-07-27T18:02:46Z

@theganyo for a high-level summary, could you edit the PR description to list the commands that are added?

theganyo · 2022-07-27T18:07:45Z

@theganyo for a high-level summary, could you edit the PR description to list the commands that are added?

done

timburks · 2022-07-27T19:55:04Z

This may be out of scope for this initial PR but...

What do we want the startup experience to be?

$ registry config list
insecure = false

Your active configuration is: "".

I think here we might want to say "you have no active configuration; see registry config configurations to see how to create and manage configurations`.

$ registry config configurations list
Error: Cannot read configs: open /home/timburks/.config/registry: no such file or directory
Cannot read configs: open /home/timburks/.config/registry: no such file or directory

Maybe: "No configuration directory exists. We expect configurations to be stored in ..."

$ registry config configurations create default
Error: Cannot create config "default": open /home/timburks/.config/registry/default: no such file or directory
Cannot create config "default": open /home/timburks/.config/registry/default: no such file or directory

Should we automatically create the needed directories?

theganyo · 2022-07-27T20:15:23Z

Yes, this PR hadn't been intended to address installation... but it's a very good question that we need to answer. I think we should think about automatically creating or having the option to create the same configurations that we put into auth/. (Also: We should definitely at least create the directory if it's missing, that was an oversight I will fix now.)

theganyo · 2022-07-27T20:22:26Z

Also: Instead of telling the user to go do another thing, how about we address it more directly like, "You have no configurations. Please enter a configuration name to create: "

theganyo · 2022-07-27T20:54:33Z

On second thought, I think I like your idea better: Directing the user to another command may not be the most convenient, but putting the client into a new mode may be jarring and could cause scripts to hang.

cmd/registry/cmd/test/test.go

seaneganx

Please remove the dead code before submitting (see the linter annotation).

seaneganx · 2022-07-28T19:41:59Z

cmd/registry/cmd/config/config_test.go

-	_, err = capture(cmd, "")
-	if err.Error() != `Config has no property "test".` {
+	want := `Config has no property "test".`
+	if err := cmd.Execute(); err.Error() != want {


Can we compare error values instead of hardcoding a string? In other words, can we make sure this test will pass even if we change the error message? Maybe that means defining type MissingPropertyError struct{field string} and checking if err != MissingPropertyError{"test"}.

theganyo added 11 commits July 19, 2022 10:49

Add CLI commands to manage config file

2b772cb

Merge remote-tracking branch 'upstream/main' into theganyo/issue649

985ac51

Add CLI commands to manage config file

547c6f5

consistently use Configs instead of Settings

9b902cc

remove unnecessary controller execution test

f044fb5

add config get, set, unset

fe07cc9

Merge remote-tracking branch 'upstream/main' into theganyo/issue649

9b70141

connection testing

d415dcc

testing

5421285

Merge branch 'main' into theganyo/issue649

dc33667

fix lint

3331500

theganyo requested review from seaneganx and timburks July 25, 2022 19:41

theganyo added 2 commits July 25, 2022 14:42

improve readme

127bbd4

Merge branch 'main' into theganyo/issue649

17088ad

seaneganx reviewed Jul 26, 2022

View reviewed changes

theganyo added 2 commits July 26, 2022 16:20

address feedback

cd1ca9f

set and use local command io rather than os io

40255be

timburks approved these changes Jul 27, 2022

View reviewed changes

create config dir as needed on config.Write

c8f63d9

seaneganx reviewed Jul 27, 2022

View reviewed changes

cmd/registry/cmd/test/test.go Outdated Show resolved Hide resolved

theganyo added 2 commits July 27, 2022 15:10

simplify Capture

b53f932

improved error messages

e29cadb

theganyo added 4 commits July 28, 2022 07:08

Merge branch 'main' into theganyo/issue649

5addda0

remove test helper package

8803957

remove capture abstraction

512a9e9

cleanup

2f7d427

seaneganx approved these changes Jul 28, 2022

View reviewed changes

test error, not message

c5715d3

theganyo merged commit 6ff58d9 into apigee:main Jul 28, 2022

theganyo deleted the theganyo/issue649 branch July 28, 2022 20:56

Add CLI commands to manage config file #671

Add CLI commands to manage config file #671

Uh oh!

Conversation

theganyo commented Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

seaneganx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

seaneganx Jul 26, 2022

Choose a reason for hiding this comment

Uh oh!

theganyo Jul 26, 2022

Choose a reason for hiding this comment

Uh oh!

theganyo Jul 28, 2022 • 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!

seaneganx Jul 26, 2022

Choose a reason for hiding this comment

Uh oh!

theganyo Jul 26, 2022 • 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!

Uh oh!

seaneganx Jul 26, 2022

Choose a reason for hiding this comment

Uh oh!

theganyo Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

timburks commented Jul 27, 2022

Uh oh!

theganyo commented Jul 27, 2022

Uh oh!

timburks commented Jul 27, 2022

Uh oh!

theganyo commented Jul 27, 2022

Uh oh!

theganyo commented Jul 27, 2022

Uh oh!

theganyo commented Jul 27, 2022

Uh oh!

Uh oh!

seaneganx left a comment

Choose a reason for hiding this comment

Uh oh!

seaneganx Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

theganyo Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theganyo commented Jul 25, 2022 •

edited

Loading

codecov bot commented Jul 25, 2022 •

edited

Loading

theganyo Jul 28, 2022 •

edited

Loading

theganyo Jul 26, 2022 •

edited

Loading