Skip to content

Commit

Permalink
Make argument and flag use consistent across commands
Browse files Browse the repository at this point in the history 
Closes #343. See the issue for the full discussion and rationale behind
the specific changes made.
  • Loading branch information
@josh-berry
josh-berry committed Mar 27, 2024
1 parent f9ac669 commit 52f90eb
Show file tree
Hide file tree
Showing 6 changed files with 294 additions and 119 deletions.
103 changes: 77 additions & 26 deletions temporalcli/commands.env.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,45 +8,75 @@ import (
"github.com/temporalio/cli/temporalcli/internal/printer"
)

func (c *TemporalEnvCommand) envNameAndKey(cctx *CommandContext, args[] string, keyFlag string) (string, string, error) {
if len(args) > 0 {
cctx.Logger.Warn("Arguments to env commands are deprecated; please use -E and/or -k instead.")

if (c.Parent.Env != "default" || keyFlag != "") {
return "", "", fmt.Errorf("cannot specify both an argument and -E/-k; please use -e and/or -k")
}

keyPieces := strings.Split(args[0], ".")
switch len(keyPieces) {
case 0:
return "", "", fmt.Errorf("no env or property name specified")
case 1:
return keyPieces[0], "", nil
case 2:
return keyPieces[0], keyPieces[1], nil
default:
return "", "", fmt.Errorf("property name may not contain dots")
}
}

if strings.Contains(keyFlag, ".") {
return "", "", fmt.Errorf("property name may not contain dots")
}

return c.Parent.Env, keyFlag, nil
}

func (c *TemporalEnvDeleteCommand) run(cctx *CommandContext, args []string) error {
keyPieces := strings.Split(args[0], ".")
if len(keyPieces) > 2 {
return fmt.Errorf("env property key to delete cannot have more than one dot")
envName, key, err := c.Parent.envNameAndKey(cctx, args, c.Key)
if err != nil {
return err
}
// Env must be present (but flag itself doesn't have to be)
env, ok := cctx.EnvConfigValues[keyPieces[0]]

// Env must be present
env, ok := cctx.EnvConfigValues[envName]
if !ok {
return fmt.Errorf("env %q not found", keyPieces[0])
return fmt.Errorf("env %q not found", envName)
}
// User can remove single flag or all in env
if len(keyPieces) > 1 {
cctx.Logger.Info("Deleting env property", "env", keyPieces[0], "property", keyPieces[1])
delete(env, keyPieces[1])
if key != "" {
cctx.Logger.Info("Deleting env property", "env", envName, "property", key)
delete(env, key)
} else {
cctx.Logger.Info("Deleting env", "env", keyPieces[0])
delete(cctx.EnvConfigValues, keyPieces[0])
cctx.Logger.Info("Deleting env", "env", env)
delete(cctx.EnvConfigValues, envName)
}
return cctx.WriteEnvConfigToFile()
}

func (c *TemporalEnvGetCommand) run(cctx *CommandContext, args []string) error {
keyPieces := strings.Split(args[0], ".")
if len(keyPieces) > 2 {
return fmt.Errorf("env key to get cannot have more than one dot")
envName, key, err := c.Parent.envNameAndKey(cctx, args, c.Key)
if err != nil {
return err
}
// Env must be present (but flag itself doesn't have to me)
env, ok := cctx.EnvConfigValues[keyPieces[0]]

// Env must be present
env, ok := cctx.EnvConfigValues[envName]
if !ok {
return fmt.Errorf("env %q not found", keyPieces[0])
return fmt.Errorf("env %q not found", envName)
}
type prop struct {
Property string `json:"property"`
Value string `json:"value"`
}
var props []prop
// User can ask for single flag or all in env
if len(keyPieces) > 1 {
props = []prop{{Property: keyPieces[1], Value: env[keyPieces[1]]}}
if key != "" {
props = []prop{{Property: key, Value: env[key]}}
} else {
props = make([]prop, 0, len(env))
for k, v := range env {
Expand All @@ -71,17 +101,38 @@ func (c *TemporalEnvListCommand) run(cctx *CommandContext, args []string) error
}

func (c *TemporalEnvSetCommand) run(cctx *CommandContext, args []string) error {
keyPieces := strings.Split(args[0], ".")
if len(keyPieces) != 2 {
return fmt.Errorf("env property key to set must have single dot separating env and value")
envName, key, err := c.Parent.envNameAndKey(cctx, args, c.Key)
if err != nil {
return err
}

if key == "" {
return fmt.Errorf("property name must be specified with -k")
}

value := c.Value
switch len(args) {
case 0:
// Use what's in the flag
case 1:
// We got an "env.name" argument passed above, but no "value" argument
return fmt.Errorf("no value provided; see --help")
case 2:
// Old-style syntax; pull the value out of the args
value = args[1]
default:
// Cobra should catch this / we should never get here; included for
// completeness anyway.
return fmt.Errorf("too many arguments provided; see --help")
}

if cctx.EnvConfigValues == nil {
cctx.EnvConfigValues = map[string]map[string]string{}
}
if cctx.EnvConfigValues[keyPieces[0]] == nil {
cctx.EnvConfigValues[keyPieces[0]] = map[string]string{}
if cctx.EnvConfigValues[envName] == nil {
cctx.EnvConfigValues[envName] = map[string]string{}
}
cctx.Logger.Info("Setting env property", "env", keyPieces[0], "property", keyPieces[1], "value", args[1])
cctx.EnvConfigValues[keyPieces[0]][keyPieces[1]] = args[1]
cctx.Logger.Info("Setting env property", "env", envName, "property", key, "value", value)
cctx.EnvConfigValues[envName][key] = value
return cctx.WriteEnvConfigToFile()
}
42 changes: 34 additions & 8 deletions temporalcli/commands.env_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ func TestEnv_Simple(t *testing.T) {

// Non-existent file, no env found for get
h.Options.EnvConfigFile = "does-not-exist"
res := h.Execute("env", "get", "myenv1")
res := h.Execute("env", "get", "-E", "myenv1")
h.ErrorContains(res.Err, `env "myenv1" not found`)

// Temp file for env
Expand All @@ -23,7 +23,7 @@ func TestEnv_Simple(t *testing.T) {
defer os.Remove(h.Options.EnvConfigFile)

// Store a key
res = h.Execute("env", "set", "myenv1.foo", "bar")
res = h.Execute("env", "set", "-E", "myenv1", "-k", "foo", "-v", "bar")
h.NoError(res.Err)
// Confirm file is YAML with expected values
b, err := os.ReadFile(h.Options.EnvConfigFile)
Expand All @@ -33,19 +33,19 @@ func TestEnv_Simple(t *testing.T) {
h.Equal("bar", yamlVals["env"]["myenv1"]["foo"])

// Store another key and another env
res = h.Execute("env", "set", "myenv1.baz", "qux")
res = h.Execute("env", "set", "-E", "myenv1", "-k", "baz", "-v", "qux")
h.NoError(res.Err)
res = h.Execute("env", "set", "myenv2.foo", "baz")
res = h.Execute("env", "set", "-E", "myenv2", "-k", "foo", "-v", "baz")
h.NoError(res.Err)

// Get single prop
res = h.Execute("env", "get", "myenv1.baz")
res = h.Execute("env", "get", "-E", "myenv1", "-k", "baz")
h.NoError(res.Err)
h.ContainsOnSameLine(res.Stdout.String(), "baz", "qux")
h.NotContains(res.Stdout.String(), "foo")

// Get all props for env
res = h.Execute("env", "get", "myenv1")
res = h.Execute("env", "get", "-E", "myenv1")
h.NoError(res.Err)
h.ContainsOnSameLine(res.Stdout.String(), "foo", "bar")
h.ContainsOnSameLine(res.Stdout.String(), "baz", "qux")
Expand All @@ -57,7 +57,7 @@ func TestEnv_Simple(t *testing.T) {
h.Contains(res.Stdout.String(), "myenv2")

// Delete single env value
res = h.Execute("env", "delete", "myenv1.foo")
res = h.Execute("env", "delete", "-E", "myenv1", "-k", "foo")
h.NoError(res.Err)
res = h.Execute("env", "get", "myenv1")
h.NoError(res.Err)
Expand All @@ -71,10 +71,36 @@ func TestEnv_Simple(t *testing.T) {
h.NotContains(res.Stdout.String(), "myenv2")

// Ensure env var overrides env file
res = h.Execute("env", "set", "myenv1.address", "something:1234")
res = h.Execute("env", "set", "-E", "myenv1", "-k", "address", "-v", "something:1234")
h.NoError(res.Err)
h.NoError(os.Setenv("TEMPORAL_ADDRESS", "overridden:1235"))
defer os.Unsetenv("TEMPORAL_ADDRESS")
res = h.Execute("workflow", "list", "--env", "myenv1")
h.Contains(res.Stderr.String(), "Env var overrode --env setting")
}

func TestEnv_InputValidation(t *testing.T) {
h := NewCommandHarness(t)
defer h.Close()

res := h.Execute("env", "get", "-E", "myenv1", "foo.bar")
h.ErrorContains(res.Err, `cannot specify both`)

res = h.Execute("env", "get", "-k", "key", "foo.bar")
h.ErrorContains(res.Err, `cannot specify both`)

res = h.Execute("env", "get", "-E", "myenv1", "-k", "foo.bar")
h.ErrorContains(res.Err, `property name may not contain dots`)

res = h.Execute("env", "set", "-E", "myenv1", "-k", "foo.bar", "-v", "")
h.ErrorContains(res.Err, `property name may not contain dots`)

res = h.Execute("env", "set", "-E", "myenv1", "-k", "", "-v", "")
h.ErrorContains(res.Err, `property name must be specified`)

res = h.Execute("env", "set", "myenv1")
h.ErrorContains(res.Err, `property name must be specified`)

res = h.Execute("env", "set", "myenv1.foo")
h.ErrorContains(res.Err, `no value provided`)
}
Loading

0 comments on commit 52f90eb

Please sign in to comment.