Skip to content

Commit

Permalink
code review fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
@lynnemorrison
lynnemorrison committed Oct 28, 2024
1 parent 780e06a commit 4484e07
Show file tree
Hide file tree
Showing 13 changed files with 174 additions and 138 deletions.
6 changes: 6 additions & 0 deletions internal/cmd/skupper/common/constants.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,9 @@ var (
ConnectorTypes = []string{"tcp"}
WorkloadTypes = []string{"deployment", "service", "daemonset", "statefulset"}
)

const (
Connectors string = "connectors"
Listeners string = "listeners"
Sites string = "sites"
)
10 changes: 9 additions & 1 deletion internal/cmd/skupper/connector/nonkube/connector_create.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package nonkube

import (
"fmt"
"net"
"strconv"

"github.com/skupperproject/skupper/internal/cmd/skupper/common"
Expand Down Expand Up @@ -54,6 +55,7 @@ func (cmd *CmdConnectorCreate) ValidateInput(args []string) []error {
numberValidator := validator.NewNumberValidator()
connectorTypeValidator := validator.NewOptionValidator(common.ConnectorTypes)
outputTypeValidator := validator.NewOptionValidator(common.OutputTypes)
hostStringValidator := validator.NewHostStringValidator()

// Validate arguments name and port
if len(args) < 2 {
Expand Down Expand Up @@ -101,7 +103,13 @@ func (cmd *CmdConnectorCreate) ValidateInput(args []string) []error {
}
}
if cmd.Flags.Host != "" {
// TBD what is valid host
ip := net.ParseIP(cmd.Flags.Host)
ok, _ := hostStringValidator.Evaluate(cmd.Flags.Host)
if !ok || ip == nil {
validationErrors = append(validationErrors, fmt.Errorf("host is not valid: a valid IP address or hostname is expected"))
}
} else {
validationErrors = append(validationErrors, fmt.Errorf("host name must be configured: an IP address or hostname is expected"))
}
if cmd.Flags.TlsSecret != "" {
// TBD what is valid TlsSecret
Expand Down
40 changes: 26 additions & 14 deletions internal/cmd/skupper/connector/nonkube/connector_create_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import (

"github.com/skupperproject/skupper/internal/cmd/skupper/common"
"github.com/skupperproject/skupper/internal/cmd/skupper/common/utils"
fs2 "github.com/skupperproject/skupper/internal/nonkube/client/fs"
"github.com/skupperproject/skupper/internal/nonkube/client/fs"
"github.com/spf13/cobra"

"gotest.tools/assert"
Expand All @@ -27,67 +27,79 @@ func TestNonKubeCmdConnectorCreate_ValidateInput(t *testing.T) {
{
name: "Connector name and port are not specified",
args: []string{},
flags: &common.CommandConnectorCreateFlags{},
flags: &common.CommandConnectorCreateFlags{Host: "1.2.3.4"},
expectedErrors: []string{"connector name and port must be configured"},
},
{
name: "Connector name is not valid",
args: []string{"my new Connector", "8080"},
flags: &common.CommandConnectorCreateFlags{},
flags: &common.CommandConnectorCreateFlags{Host: "1.2.3.4"},
expectedErrors: []string{"connector name is not valid: value does not match this regular expression: ^[a-z0-9]([-a-z0-9]*[a-z0-9])*(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])*)*$"},
},
{
name: "Connector name is empty",
args: []string{"", "1234"},
flags: &common.CommandConnectorCreateFlags{},
flags: &common.CommandConnectorCreateFlags{Host: "1.2.3.4"},
expectedErrors: []string{"connector name must not be empty"},
},
{
name: "connector port empty",
args: []string{"my-name-port-empty", ""},
flags: &common.CommandConnectorCreateFlags{},
flags: &common.CommandConnectorCreateFlags{Host: "1.2.3.4"},
expectedErrors: []string{"connector port must not be empty"},
},
{
name: "port is not valid",
args: []string{"my-connector-port", "abcd"},
flags: &common.CommandConnectorCreateFlags{},
flags: &common.CommandConnectorCreateFlags{Host: "1.2.3.4"},
expectedErrors: []string{"connector port is not valid: strconv.Atoi: parsing \"abcd\": invalid syntax"},
},
{
name: "port not positive",
args: []string{"my-port-positive", "-45"},
flags: &common.CommandConnectorCreateFlags{},
flags: &common.CommandConnectorCreateFlags{Host: "1.2.3.4"},
expectedErrors: []string{"connector port is not valid: value is not positive"},
},
{
name: "more than two arguments was specified",
args: []string{"my", "Connector", "test"},
flags: &common.CommandConnectorCreateFlags{},
flags: &common.CommandConnectorCreateFlags{Host: "1.2.3.4"},
expectedErrors: []string{"only two arguments are allowed for this command"},
},
{
name: "type is not valid",
args: []string{"my-connector", "8080"},
flags: &common.CommandConnectorCreateFlags{ConnectorType: "not-valid"},
flags: &common.CommandConnectorCreateFlags{ConnectorType: "not-valid", Host: "1.2.3.4"},
expectedErrors: []string{"connector type is not valid: value not-valid not allowed. It should be one of this options: [tcp]"},
},
{
name: "routing key is not valid",
args: []string{"my-connector-rk", "8080"},
flags: &common.CommandConnectorCreateFlags{RoutingKey: "not-valid$"},
flags: &common.CommandConnectorCreateFlags{RoutingKey: "not-valid$", Host: "1.2.3.4"},
expectedErrors: []string{"routing key is not valid: value does not match this regular expression: ^[a-z0-9]([-a-z0-9]*[a-z0-9])*(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])*)*$"},
},
{
name: "host is not valid",
args: []string{"my-connector-host", "8080"},
flags: &common.CommandConnectorCreateFlags{Host: "not-valid$"},
expectedErrors: []string{"host is not valid: a valid IP address or hostname is expected"},
},
{
name: "host is not configued",
args: []string{"my-connector-host", "8080"},
flags: &common.CommandConnectorCreateFlags{},
expectedErrors: []string{"host name must be configured: an IP address or hostname is expected"},
},
{
name: "output format is not valid",
args: []string{"my-connector", "8080"},
flags: &common.CommandConnectorCreateFlags{Output: "not-valid"},
flags: &common.CommandConnectorCreateFlags{Output: "not-valid", Host: "1.2.3.4"},
expectedErrors: []string{"output type is not valid: value not-valid not allowed. It should be one of this options: [json yaml]"},
},
{
name: "kubernetes flags are not valid on this platform",
args: []string{"my-connector", "8080"},
flags: &common.CommandConnectorCreateFlags{},
flags: &common.CommandConnectorCreateFlags{Host: "1.2.3.4"},
expectedErrors: []string{},
cobraGenericFlags: map[string]string{
common.FlagNameContext: "test",
Expand Down Expand Up @@ -190,7 +202,7 @@ func TestNonKubeCmdConnectorCreate_InputToOptions(t *testing.T) {
cmd.Flags = &test.flags
cmd.connectorName = "my-Connector"
cmd.namespace = test.namespace
cmd.connectorHandler = fs2.NewConnectorHandler(cmd.namespace)
cmd.connectorHandler = fs.NewConnectorHandler(cmd.namespace)

cmd.InputToOptions()

Expand Down Expand Up @@ -266,7 +278,7 @@ func TestNonKubeCmdConnectorCreate_Run(t *testing.T) {
command.host = test.host
command.connectorType = test.connectorType
command.namespace = test.namespace
command.connectorHandler = fs2.NewConnectorHandler(command.namespace)
command.connectorHandler = fs.NewConnectorHandler(command.namespace)
defer command.connectorHandler.Delete("test1")
t.Run(test.name, func(t *testing.T) {

Expand Down
4 changes: 2 additions & 2 deletions internal/cmd/skupper/connector/nonkube/connector_delete_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import (

"github.com/skupperproject/skupper/internal/cmd/skupper/common"
"github.com/skupperproject/skupper/internal/cmd/skupper/common/utils"
fs2 "github.com/skupperproject/skupper/internal/nonkube/client/fs"
"github.com/skupperproject/skupper/internal/nonkube/client/fs"
"github.com/spf13/cobra"

"gotest.tools/assert"
Expand Down Expand Up @@ -119,7 +119,7 @@ func TestCmdConnectorDelete_Run(t *testing.T) {

cmd.connectorName = test.deleteName
cmd.namespace = test.namespace
cmd.connectorHandler = fs2.NewConnectorHandler(cmd.namespace)
cmd.connectorHandler = fs.NewConnectorHandler(cmd.namespace)
cmd.InputToOptions()

err := cmd.Run()
Expand Down
26 changes: 7 additions & 19 deletions internal/cmd/skupper/connector/nonkube/connector_status.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,22 +75,15 @@ func (cmd *CmdConnectorStatus) ValidateInput(args []string) []error {

func (cmd *CmdConnectorStatus) Run() error {
if cmd.connectorName == "" {
_, connectors, err := cmd.connectorHandler.GetRuntime("")
if err != nil {
fmt.Println("failed getting directory")
connectors, err := cmd.connectorHandler.List()
if connectors == nil || err != nil {
fmt.Println("No connectors found:")
return err
}
if cmd.output != "" {
for _, file := range connectors {
fmt.Println("file name:", file.Name())
connector, _, err := cmd.connectorHandler.GetRuntime(file.Name())
if err != nil {
fmt.Println("failed getting connector")
return err
}
for _, connector := range connectors {
encodedOutput, err := utils.Encode(cmd.output, connector)
if err != nil {
fmt.Println("failed encoding connector output")
return err
}
fmt.Println(encodedOutput)
Expand All @@ -99,12 +92,7 @@ func (cmd *CmdConnectorStatus) Run() error {
tw := tabwriter.NewWriter(os.Stdout, 8, 8, 1, '\t', tabwriter.TabIndent)
_, _ = fmt.Fprintln(tw, fmt.Sprintf("%s\t%s\t%s\t%s\t%s",
"NAME", "STATUS", "ROUTING-KEY", "HOST", "PORT"))
for _, file := range connectors {
connector, _, err := cmd.connectorHandler.GetRuntime(file.Name())
if err != nil {
fmt.Println("failed getting connector")
return err
}
for _, connector := range connectors {
status := "Not Ready"
if connector.IsConfigured() {
status = "Ok"
Expand All @@ -115,9 +103,9 @@ func (cmd *CmdConnectorStatus) Run() error {
_ = tw.Flush()
}
} else {
connector, _, err := cmd.connectorHandler.GetRuntime(cmd.connectorName + ".yaml")
connector, err := cmd.connectorHandler.Get(cmd.connectorName)
if connector == nil || err != nil {
fmt.Println("No connectors found:", err)
fmt.Println("No connectors found:")
return err
}
if cmd.output != "" {
Expand Down
28 changes: 18 additions & 10 deletions internal/cmd/skupper/connector/nonkube/connector_status_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@ package nonkube

import (
"os"
"path/filepath"
"testing"

"github.com/skupperproject/skupper/internal/cmd/skupper/common"
"github.com/skupperproject/skupper/internal/cmd/skupper/common/utils"
fs2 "github.com/skupperproject/skupper/internal/nonkube/client/fs"
"github.com/skupperproject/skupper/internal/nonkube/client/fs"
"github.com/skupperproject/skupper/pkg/apis/skupper/v1alpha1"
"gotest.tools/assert"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -24,6 +25,10 @@ func TestCmdConnectorStatus_ValidateInput(t *testing.T) {
expectedErrors []string
}

homeDir, err := os.UserHomeDir()
assert.Check(t, err == nil)
path := filepath.Join(homeDir, "/.local/share/skupper/namespaces/test/runtime/state")

testTable := []test{
{
name: "connector is not shown because connector does not exist in the namespace",
Expand Down Expand Up @@ -82,10 +87,12 @@ func TestCmdConnectorStatus_ValidateInput(t *testing.T) {

command := &CmdConnectorStatus{}
command.namespace = "test"
command.connectorHandler = fs2.NewConnectorHandler(command.namespace)
command.connectorHandler = fs.NewConnectorHandler(command.namespace)

defer command.connectorHandler.Delete("my-connector")
err := command.connectorHandler.Add(connectorResource)
content, err := command.connectorHandler.EncodeToYaml(connectorResource)
assert.Check(t, err == nil)
err = command.connectorHandler.WriteFile(path, "my-connector.yaml", content, common.Connectors)
assert.Check(t, err == nil)

for _, test := range testTable {
Expand Down Expand Up @@ -125,13 +132,13 @@ func TestCmdConnectorStatus_Run(t *testing.T) {

homeDir, err := os.UserHomeDir()
assert.Check(t, err == nil)
path := filepath.Join(homeDir, "/.local/share/skupper/namespaces/test/")

testTable := []test{
{
name: "run fails connector doesn't exist",
connectorName: "no-connector",
errorMessage: "failed to read file: open " + homeDir + "/.local/share/skupper/namespaces/test/runtime/state/connectors/no-connector.yaml: no such file or directory",
},
errorMessage: "failed to read file: open " + path + "/input/sources/connectors/no-connector.yaml: no such file or directory"},
{
name: "runs ok, returns 1 connectors",
connectorName: "my-connector",
Expand Down Expand Up @@ -216,19 +223,19 @@ func TestCmdConnectorStatus_Run(t *testing.T) {
// add two connectors in runtime directory
command := &CmdConnectorStatus{}
command.namespace = "test"
command.connectorHandler = fs2.NewConnectorHandler(command.namespace)
command.connectorHandler = fs.NewConnectorHandler(command.namespace)

defer command.connectorHandler.Delete("my-connector")
defer command.connectorHandler.Delete("my-connector2")

content, err := command.connectorHandler.EncodeToYaml(connectorResource1)
assert.Check(t, err == nil)
err = command.connectorHandler.WriteFile(".local/share/skupper/namespaces/test/runtime/state", "my-connector.yaml", content, "connectors")
err = command.connectorHandler.WriteFile(path+"/runtime/state", "my-connector.yaml", content, common.Connectors)
assert.Check(t, err == nil)

content, err = command.connectorHandler.EncodeToYaml(connectorResource2)
assert.Check(t, err == nil)
err = command.connectorHandler.WriteFile(".local/share/skupper/namespaces/test/runtime/state", "my-connector2.yaml", content, "connectors")
err = command.connectorHandler.WriteFile(path+"/runtime/state", "my-connector2.yaml", content, common.Connectors)
assert.Check(t, err == nil)

for _, test := range testTable {
Expand Down Expand Up @@ -260,18 +267,19 @@ func TestCmdConnectorStatus_RunNoDirectory(t *testing.T) {

homeDir, err := os.UserHomeDir()
assert.Check(t, err == nil)
path := filepath.Join(homeDir, "/.local/share/skupper/namespaces/default/input/sources/connectors")

testTable := []test{
{
name: "runs fails no directory",
errorMessage: "failed to read directory: open " + homeDir + "/.local/share/skupper/namespaces/default/runtime/state/connectors: no such file or directory",
errorMessage: "failed to read directory: open " + path + ": no such file or directory",
},
}

for _, test := range testTable {
command := &CmdConnectorStatus{}
command.namespace = "default"
command.connectorHandler = fs2.NewConnectorHandler(command.namespace)
command.connectorHandler = fs.NewConnectorHandler(command.namespace)
command.connectorName = test.connectorName
command.Flags = &test.flags
command.output = command.Flags.Output
Expand Down
11 changes: 9 additions & 2 deletions internal/cmd/skupper/connector/nonkube/connector_update.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package nonkube

import (
"fmt"
"net"

"github.com/skupperproject/skupper/internal/cmd/skupper/common"
"github.com/skupperproject/skupper/internal/cmd/skupper/common/utils"
Expand Down Expand Up @@ -47,6 +48,7 @@ func (cmd *CmdConnectorUpdate) ValidateInput(args []string) []error {
numberValidator := validator.NewNumberValidator()
connectorTypeValidator := validator.NewOptionValidator(common.ConnectorTypes)
outputTypeValidator := validator.NewOptionValidator(common.OutputTypes)
hostStringValidator := validator.NewHostStringValidator()

if cmd.CobraCmd != nil && cmd.CobraCmd.Flag(common.FlagNameContext) != nil && cmd.CobraCmd.Flag(common.FlagNameContext).Value.String() != "" {
fmt.Println("Warning: --context flag is not supported on this platform")
Expand Down Expand Up @@ -113,8 +115,13 @@ func (cmd *CmdConnectorUpdate) ValidateInput(args []string) []error {
}
}
if cmd.Flags.Host != "" {
//TBD what characters are not allowed for host flag
cmd.newSettings.host = cmd.Flags.Host
ip := net.ParseIP(cmd.Flags.Host)
ok, _ := hostStringValidator.Evaluate(cmd.Flags.Host)
if !ok || ip == nil {
validationErrors = append(validationErrors, fmt.Errorf("host is not valid: a valid IP address or hostname is expected"))
} else {
cmd.newSettings.host = cmd.Flags.Host
}
}
if cmd.Flags.Output != "" {
ok, err := outputTypeValidator.Evaluate(cmd.Flags.Output)
Expand Down
Loading

0 comments on commit 4484e07

Please sign in to comment.