Merge pull request #2319 from cli/cleanup-duplicate-config

Refactor config command to improve testability
This commit is contained in:
Nate Smith 2020-11-03 12:54:16 -06:00 committed by GitHub
commit 441f40b427
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
9 changed files with 550 additions and 308 deletions

View file

@ -46,9 +46,45 @@ func ConfigOptions() []ConfigOption {
return configOptions
}
var configValues = map[string][]string{
"git_protocol": {"ssh", "https"},
"prompt": {"enabled", "disabled"},
func ValidateKey(key string) error {
for _, configKey := range configOptions {
if key == configKey.Key {
return nil
}
}
return fmt.Errorf("invalid key")
}
type InvalidValueError struct {
ValidValues []string
}
func (e InvalidValueError) Error() string {
return "invalid value"
}
func ValidateValue(key, value string) error {
var validValues []string
for _, v := range configOptions {
if v.Key == key {
validValues = v.AllowedValues
break
}
}
if validValues == nil {
return nil
}
for _, v := range validValues {
if v == value {
return nil
}
}
return &InvalidValueError{ValidValues: validValues}
}
// This interface describes interacting with some persistent configuration for gh.
@ -306,33 +342,7 @@ func (c *fileConfig) GetWithSource(hostname, key string) (string, string, error)
return value, defaultSource, nil
}
type InvalidValueError struct {
ValidValues []string
}
func (e InvalidValueError) Error() string {
return "invalid value"
}
func validateConfigEntry(key, value string) error {
validValues, found := configValues[key]
if !found {
return nil
}
for _, v := range validValues {
if v == value {
return nil
}
}
return &InvalidValueError{ValidValues: validValues}
}
func (c *fileConfig) Set(hostname, key, value string) error {
if err := validateConfigEntry(key, value); err != nil {
return err
}
if hostname == "" {
return c.SetStringValue(key, value)
} else {

View file

@ -28,7 +28,6 @@ func Test_fileConfig_Set(t *testing.T) {
example.com:
editor: vim
`, hostsBuf.String())
assert.EqualError(t, c.Set("github.com", "git_protocol", "sshpps"), "invalid value")
}
func Test_defaultConfig(t *testing.T) {
@ -70,16 +69,33 @@ func Test_defaultConfig(t *testing.T) {
assert.Equal(t, expansion, "pr checkout")
}
func Test_validateConfigEntry(t *testing.T) {
err := validateConfigEntry("git_protocol", "sshpps")
func Test_ValidateValue(t *testing.T) {
err := ValidateValue("git_protocol", "sshpps")
assert.EqualError(t, err, "invalid value")
err = validateConfigEntry("git_protocol", "ssh")
err = ValidateValue("git_protocol", "ssh")
assert.Nil(t, err)
err = validateConfigEntry("editor", "vim")
err = ValidateValue("editor", "vim")
assert.Nil(t, err)
err = validateConfigEntry("got", "123")
err = ValidateValue("got", "123")
assert.Nil(t, err)
}
func Test_ValidateKey(t *testing.T) {
err := ValidateKey("invalid")
assert.EqualError(t, err, "invalid key")
err = ValidateKey("git_protocol")
assert.NoError(t, err)
err = ValidateKey("editor")
assert.NoError(t, err)
err = ValidateKey("prompt")
assert.NoError(t, err)
err = ValidateKey("pager")
assert.NoError(t, err)
}

51
internal/config/stub.go Normal file
View file

@ -0,0 +1,51 @@
package config
import (
"errors"
)
type ConfigStub map[string]string
func genKey(host, key string) string {
if host != "" {
return host + ":" + key
}
return key
}
func (c ConfigStub) Get(host, key string) (string, error) {
val, _, err := c.GetWithSource(host, key)
return val, err
}
func (c ConfigStub) GetWithSource(host, key string) (string, string, error) {
if v, found := c[genKey(host, key)]; found {
return v, "(memory)", nil
}
return "", "", errors.New("not found")
}
func (c ConfigStub) Set(host, key, value string) error {
c[genKey(host, key)] = value
return nil
}
func (c ConfigStub) Aliases() (*AliasConfig, error) {
return nil, nil
}
func (c ConfigStub) Hosts() ([]string, error) {
return nil, nil
}
func (c ConfigStub) UnsetHost(hostname string) {
}
func (c ConfigStub) CheckWriteable(host, key string) error {
return nil
}
func (c ConfigStub) Write() error {
c["_written"] = "true"
return nil
}

View file

@ -1,12 +1,12 @@
package config
import (
"errors"
"fmt"
"strings"
"github.com/MakeNowJust/heredoc"
"github.com/cli/cli/internal/config"
cmdGet "github.com/cli/cli/pkg/cmd/config/get"
cmdSet "github.com/cli/cli/pkg/cmd/config/set"
"github.com/cli/cli/pkg/cmdutil"
"github.com/spf13/cobra"
)
@ -31,100 +31,8 @@ func NewCmdConfig(f *cmdutil.Factory) *cobra.Command {
cmdutil.DisableAuthCheck(cmd)
cmd.AddCommand(NewCmdConfigGet(f))
cmd.AddCommand(NewCmdConfigSet(f))
return cmd
}
func NewCmdConfigGet(f *cmdutil.Factory) *cobra.Command {
var hostname string
cmd := &cobra.Command{
Use: "get <key>",
Short: "Print the value of a given configuration key",
Example: heredoc.Doc(`
$ gh config get git_protocol
https
`),
Args: cobra.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
cfg, err := f.Config()
if err != nil {
return err
}
val, err := cfg.Get(hostname, args[0])
if err != nil {
return err
}
if val != "" {
fmt.Fprintf(f.IOStreams.Out, "%s\n", val)
}
return nil
},
}
cmd.Flags().StringVarP(&hostname, "host", "h", "", "Get per-host setting")
return cmd
}
func NewCmdConfigSet(f *cmdutil.Factory) *cobra.Command {
var hostname string
cmd := &cobra.Command{
Use: "set <key> <value>",
Short: "Update configuration with a value for the given key",
Example: heredoc.Doc(`
$ gh config set editor vim
$ gh config set editor "code --wait"
$ gh config set git_protocol ssh --host github.com
$ gh config set prompt disabled
`),
Args: cobra.ExactArgs(2),
RunE: func(cmd *cobra.Command, args []string) error {
cfg, err := f.Config()
if err != nil {
return err
}
key, value := args[0], args[1]
knownKey := false
for _, configKey := range config.ConfigOptions() {
if key == configKey.Key {
knownKey = true
break
}
}
if !knownKey {
iostreams := f.IOStreams
warningIcon := iostreams.ColorScheme().WarningIcon()
fmt.Fprintf(iostreams.ErrOut, "%s warning: '%s' is not a known configuration key\n", warningIcon, key)
}
err = cfg.Set(hostname, key, value)
if err != nil {
var invalidValue *config.InvalidValueError
if errors.As(err, &invalidValue) {
var values []string
for _, v := range invalidValue.ValidValues {
values = append(values, fmt.Sprintf("'%s'", v))
}
return fmt.Errorf("failed to set %q to %q: valid values are %v", key, value, strings.Join(values, ", "))
}
return fmt.Errorf("failed to set %q to %q: %w", key, value, err)
}
err = cfg.Write()
if err != nil {
return fmt.Errorf("failed to write config to disk: %w", err)
}
return nil
},
}
cmd.Flags().StringVarP(&hostname, "host", "h", "", "Set per-host setting")
cmd.AddCommand(cmdGet.NewCmdConfigGet(f, nil))
cmd.AddCommand(cmdSet.NewCmdConfigSet(f, nil))
return cmd
}

View file

@ -1,177 +0,0 @@
package config
import (
"errors"
"testing"
"github.com/cli/cli/internal/config"
"github.com/cli/cli/pkg/cmdutil"
"github.com/cli/cli/pkg/iostreams"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
type configStub map[string]string
func genKey(host, key string) string {
if host != "" {
return host + ":" + key
}
return key
}
func (c configStub) Get(host, key string) (string, error) {
val, _, err := c.GetWithSource(host, key)
return val, err
}
func (c configStub) GetWithSource(host, key string) (string, string, error) {
if v, found := c[genKey(host, key)]; found {
return v, "(memory)", nil
}
return "", "", errors.New("not found")
}
func (c configStub) Set(host, key, value string) error {
c[genKey(host, key)] = value
return nil
}
func (c configStub) Aliases() (*config.AliasConfig, error) {
return nil, nil
}
func (c configStub) Hosts() ([]string, error) {
return nil, nil
}
func (c configStub) UnsetHost(hostname string) {
}
func (c configStub) CheckWriteable(host, key string) error {
return nil
}
func (c configStub) Write() error {
c["_written"] = "true"
return nil
}
func TestConfigGet(t *testing.T) {
tests := []struct {
name string
config configStub
args []string
stdout string
stderr string
}{
{
name: "get key",
config: configStub{
"editor": "ed",
},
args: []string{"editor"},
stdout: "ed\n",
stderr: "",
},
{
name: "get key scoped by host",
config: configStub{
"editor": "ed",
"github.com:editor": "vim",
},
args: []string{"editor", "-h", "github.com"},
stdout: "vim\n",
stderr: "",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
io, _, stdout, stderr := iostreams.Test()
f := &cmdutil.Factory{
IOStreams: io,
Config: func() (config.Config, error) {
return tt.config, nil
},
}
cmd := NewCmdConfigGet(f)
cmd.Flags().BoolP("help", "x", false, "")
cmd.SetArgs(tt.args)
cmd.SetOut(stdout)
cmd.SetErr(stderr)
_, err := cmd.ExecuteC()
require.NoError(t, err)
assert.Equal(t, tt.stdout, stdout.String())
assert.Equal(t, tt.stderr, stderr.String())
assert.Equal(t, "", tt.config["_written"])
})
}
}
func TestConfigSet(t *testing.T) {
tests := []struct {
name string
config configStub
args []string
expectKey string
expectVal string
stdout string
stderr string
}{
{
name: "set key",
config: configStub{},
args: []string{"editor", "vim"},
expectKey: "editor",
expectVal: "vim",
stdout: "",
stderr: "",
},
{
name: "set key scoped by host",
config: configStub{},
args: []string{"editor", "vim", "-h", "github.com"},
expectKey: "github.com:editor",
expectVal: "vim",
stdout: "",
stderr: "",
},
{
name: "set key",
config: configStub{},
args: []string{"unknownKey", "someValue"},
expectKey: "unknownKey",
expectVal: "someValue",
stdout: "",
stderr: "! warning: 'unknownKey' is not a known configuration key\n",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
io, _, stdout, stderr := iostreams.Test()
f := &cmdutil.Factory{
IOStreams: io,
Config: func() (config.Config, error) {
return tt.config, nil
},
}
cmd := NewCmdConfigSet(f)
cmd.Flags().BoolP("help", "x", false, "")
cmd.SetArgs(tt.args)
cmd.SetOut(stdout)
cmd.SetErr(stderr)
_, err := cmd.ExecuteC()
require.NoError(t, err)
assert.Equal(t, tt.stdout, stdout.String())
assert.Equal(t, tt.stderr, stderr.String())
assert.Equal(t, tt.expectVal, tt.config[tt.expectKey])
assert.Equal(t, "true", tt.config["_written"])
})
}
}

65
pkg/cmd/config/get/get.go Normal file
View file

@ -0,0 +1,65 @@
package get
import (
"fmt"
"github.com/MakeNowJust/heredoc"
"github.com/cli/cli/internal/config"
"github.com/cli/cli/pkg/cmdutil"
"github.com/cli/cli/pkg/iostreams"
"github.com/spf13/cobra"
)
type GetOptions struct {
IO *iostreams.IOStreams
Config config.Config
Hostname string
Key string
}
func NewCmdConfigGet(f *cmdutil.Factory, runF func(*GetOptions) error) *cobra.Command {
opts := &GetOptions{
IO: f.IOStreams,
}
cmd := &cobra.Command{
Use: "get <key>",
Short: "Print the value of a given configuration key",
Example: heredoc.Doc(`
$ gh config get git_protocol
https
`),
Args: cobra.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
config, err := f.Config()
if err != nil {
return err
}
opts.Config = config
opts.Key = args[0]
if runF != nil {
return runF(opts)
}
return getRun(opts)
},
}
cmd.Flags().StringVarP(&opts.Hostname, "host", "h", "", "Get per-host setting")
return cmd
}
func getRun(opts *GetOptions) error {
val, err := opts.Config.Get(opts.Hostname, opts.Key)
if err != nil {
return err
}
if val != "" {
fmt.Fprintf(opts.IO.Out, "%s\n", val)
}
return nil
}

View file

@ -0,0 +1,122 @@
package get
import (
"bytes"
"testing"
"github.com/cli/cli/internal/config"
"github.com/cli/cli/pkg/cmdutil"
"github.com/cli/cli/pkg/iostreams"
"github.com/google/shlex"
"github.com/stretchr/testify/assert"
)
func TestNewCmdConfigGet(t *testing.T) {
tests := []struct {
name string
input string
output GetOptions
wantsErr bool
}{
{
name: "no arguments",
input: "",
output: GetOptions{},
wantsErr: true,
},
{
name: "get key",
input: "key",
output: GetOptions{Key: "key"},
wantsErr: false,
},
{
name: "get key with host",
input: "key --host test.com",
output: GetOptions{Hostname: "test.com", Key: "key"},
wantsErr: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
f := &cmdutil.Factory{
Config: func() (config.Config, error) {
return config.ConfigStub{}, nil
},
}
argv, err := shlex.Split(tt.input)
assert.NoError(t, err)
var gotOpts *GetOptions
cmd := NewCmdConfigGet(f, func(opts *GetOptions) error {
gotOpts = opts
return nil
})
cmd.Flags().BoolP("help", "x", false, "")
cmd.SetArgs(argv)
cmd.SetIn(&bytes.Buffer{})
cmd.SetOut(&bytes.Buffer{})
cmd.SetErr(&bytes.Buffer{})
_, err = cmd.ExecuteC()
if tt.wantsErr {
assert.Error(t, err)
return
}
assert.NoError(t, err)
assert.Equal(t, tt.output.Hostname, gotOpts.Hostname)
assert.Equal(t, tt.output.Key, gotOpts.Key)
})
}
}
func Test_getRun(t *testing.T) {
tests := []struct {
name string
input *GetOptions
stdout string
stderr string
wantErr bool
}{
{
name: "get key",
input: &GetOptions{
Key: "editor",
Config: config.ConfigStub{
"editor": "ed",
},
},
stdout: "ed\n",
},
{
name: "get key scoped by host",
input: &GetOptions{
Hostname: "github.com",
Key: "editor",
Config: config.ConfigStub{
"editor": "ed",
"github.com:editor": "vim",
},
},
stdout: "vim\n",
},
}
for _, tt := range tests {
io, _, stdout, stderr := iostreams.Test()
tt.input.IO = io
t.Run(tt.name, func(t *testing.T) {
err := getRun(tt.input)
assert.NoError(t, err)
assert.Equal(t, tt.stdout, stdout.String())
assert.Equal(t, tt.stderr, stderr.String())
_, err = tt.input.Config.Get("", "_written")
assert.Error(t, err)
})
}
}

90
pkg/cmd/config/set/set.go Normal file
View file

@ -0,0 +1,90 @@
package set
import (
"errors"
"fmt"
"strings"
"github.com/MakeNowJust/heredoc"
"github.com/cli/cli/internal/config"
"github.com/cli/cli/pkg/cmdutil"
"github.com/cli/cli/pkg/iostreams"
"github.com/spf13/cobra"
)
type SetOptions struct {
IO *iostreams.IOStreams
Config config.Config
Key string
Value string
Hostname string
}
func NewCmdConfigSet(f *cmdutil.Factory, runF func(*SetOptions) error) *cobra.Command {
opts := &SetOptions{
IO: f.IOStreams,
}
cmd := &cobra.Command{
Use: "set <key> <value>",
Short: "Update configuration with a value for the given key",
Example: heredoc.Doc(`
$ gh config set editor vim
$ gh config set editor "code --wait"
$ gh config set git_protocol ssh --host github.com
$ gh config set prompt disabled
`),
Args: cobra.ExactArgs(2),
RunE: func(cmd *cobra.Command, args []string) error {
config, err := f.Config()
if err != nil {
return err
}
opts.Config = config
opts.Key = args[0]
opts.Value = args[1]
if runF != nil {
return runF(opts)
}
return setRun(opts)
},
}
cmd.Flags().StringVarP(&opts.Hostname, "host", "h", "", "Set per-host setting")
return cmd
}
func setRun(opts *SetOptions) error {
err := config.ValidateKey(opts.Key)
if err != nil {
warningIcon := opts.IO.ColorScheme().WarningIcon()
fmt.Fprintf(opts.IO.ErrOut, "%s warning: '%s' is not a known configuration key\n", warningIcon, opts.Key)
}
err = config.ValidateValue(opts.Key, opts.Value)
if err != nil {
var invalidValue *config.InvalidValueError
if errors.As(err, &invalidValue) {
var values []string
for _, v := range invalidValue.ValidValues {
values = append(values, fmt.Sprintf("'%s'", v))
}
return fmt.Errorf("failed to set %q to %q: valid values are %v", opts.Key, opts.Value, strings.Join(values, ", "))
}
}
err = opts.Config.Set(opts.Hostname, opts.Key, opts.Value)
if err != nil {
return fmt.Errorf("failed to set %q to %q: %w", opts.Key, opts.Value, err)
}
err = opts.Config.Write()
if err != nil {
return fmt.Errorf("failed to write config to disk: %w", err)
}
return nil
}

View file

@ -0,0 +1,157 @@
package set
import (
"bytes"
"testing"
"github.com/cli/cli/internal/config"
"github.com/cli/cli/pkg/cmdutil"
"github.com/cli/cli/pkg/iostreams"
"github.com/google/shlex"
"github.com/stretchr/testify/assert"
)
func TestNewCmdConfigSet(t *testing.T) {
tests := []struct {
name string
input string
output SetOptions
wantsErr bool
}{
{
name: "no arguments",
input: "",
output: SetOptions{},
wantsErr: true,
},
{
name: "no value argument",
input: "key",
output: SetOptions{},
wantsErr: true,
},
{
name: "set key value",
input: "key value",
output: SetOptions{Key: "key", Value: "value"},
wantsErr: false,
},
{
name: "set key value with host",
input: "key value --host test.com",
output: SetOptions{Hostname: "test.com", Key: "key", Value: "value"},
wantsErr: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
f := &cmdutil.Factory{
Config: func() (config.Config, error) {
return config.ConfigStub{}, nil
},
}
argv, err := shlex.Split(tt.input)
assert.NoError(t, err)
var gotOpts *SetOptions
cmd := NewCmdConfigSet(f, func(opts *SetOptions) error {
gotOpts = opts
return nil
})
cmd.Flags().BoolP("help", "x", false, "")
cmd.SetArgs(argv)
cmd.SetIn(&bytes.Buffer{})
cmd.SetOut(&bytes.Buffer{})
cmd.SetErr(&bytes.Buffer{})
_, err = cmd.ExecuteC()
if tt.wantsErr {
assert.Error(t, err)
return
}
assert.NoError(t, err)
assert.Equal(t, tt.output.Hostname, gotOpts.Hostname)
assert.Equal(t, tt.output.Key, gotOpts.Key)
assert.Equal(t, tt.output.Value, gotOpts.Value)
})
}
}
func Test_setRun(t *testing.T) {
tests := []struct {
name string
input *SetOptions
expectedValue string
stdout string
stderr string
wantsErr bool
errMsg string
}{
{
name: "set key value",
input: &SetOptions{
Config: config.ConfigStub{},
Key: "editor",
Value: "vim",
},
expectedValue: "vim",
},
{
name: "set key value scoped by host",
input: &SetOptions{
Config: config.ConfigStub{},
Hostname: "github.com",
Key: "editor",
Value: "vim",
},
expectedValue: "vim",
},
{
name: "set unknown key",
input: &SetOptions{
Config: config.ConfigStub{},
Key: "unknownKey",
Value: "someValue",
},
expectedValue: "someValue",
stderr: "! warning: 'unknownKey' is not a known configuration key\n",
},
{
name: "set invalid value",
input: &SetOptions{
Config: config.ConfigStub{},
Key: "git_protocol",
Value: "invalid",
},
wantsErr: true,
errMsg: "failed to set \"git_protocol\" to \"invalid\": valid values are 'https', 'ssh'",
},
}
for _, tt := range tests {
io, _, stdout, stderr := iostreams.Test()
tt.input.IO = io
t.Run(tt.name, func(t *testing.T) {
err := setRun(tt.input)
if tt.wantsErr {
assert.EqualError(t, err, tt.errMsg)
return
}
assert.NoError(t, err)
assert.Equal(t, tt.stdout, stdout.String())
assert.Equal(t, tt.stderr, stderr.String())
val, err := tt.input.Config.Get(tt.input.Hostname, tt.input.Key)
assert.NoError(t, err)
assert.Equal(t, tt.expectedValue, val)
val, err = tt.input.Config.Get("", "_written")
assert.NoError(t, err)
assert.Equal(t, "true", val)
})
}
}