fix(nodebuilder/core): non default values from config.toml are written over by flag defaults in specific circumstances

cmd/flags_node.go

@@ -53,6 +53,7 @@ func ParseNodeFlags(ctx context.Context, cmd *cobra.Command, network p2p.Network
 	if nodeConfig != "" {
 		// try to load config from given path
 		cfg, err := nodebuilder.LoadConfig(nodeConfig)
+
 		if err != nil {
 			return ctx, fmt.Errorf("cmd: while parsing '%s': %w", nodeConfigFlag, err)
 		}

nodebuilder/core/config.go

@@ -7,6 +7,11 @@ import (
 	"github.com/celestiaorg/celestia-node/libs/utils"
 )
 
+const (
+	DefaultRPCPort  = "26657"
+	DefaultGRPCPort = "9090"
+)
+
 var MetricsEnabled bool
 
 // Config combines all configuration fields for managing the relationship with a Core node.
@@ -21,8 +26,8 @@ type Config struct {
 func DefaultConfig() Config {
 	return Config{
 		IP:       "",
-		RPCPort:  "26657",
-		GRPCPort: "9090",
+		RPCPort:  DefaultRPCPort,
+		GRPCPort: DefaultGRPCPort,
 	}
 }
 

nodebuilder/core/flags.go

@@ -26,12 +26,12 @@ func Flags() *flag.FlagSet {
 	)
 	flags.String(
 		coreRPCFlag,
-		"26657",
+		DefaultRPCPort,
 		"Set a custom RPC port for the core node connection. The --core.ip flag must also be provided.",
 	)
 	flags.String(
 		coreGRPCFlag,
-		"9090",
+		DefaultGRPCPort,
 		"Set a custom gRPC port for the core node connection. The --core.ip flag must also be provided.",
 	)
 	return flags
@@ -50,11 +50,18 @@ func ParseFlags(
 		return nil
 	}
 
-	rpc := cmd.Flag(coreRPCFlag).Value.String()
-	grpc := cmd.Flag(coreGRPCFlag).Value.String()
+	// in the case that the RPC port is not set, use the default port
+	// or in the case a value has been set via flag,
+	// set that as highest precedence
+	if cfg.RPCPort == "" || cmd.Flag(coreRPCFlag).Changed {
+		rpc := cmd.Flag(coreRPCFlag).Value.String()
+		cfg.RPCPort = rpc
+	}
+	if cfg.GRPCPort == "" || cmd.Flag(coreGRPCFlag).Changed {
+		grpc := cmd.Flag(coreGRPCFlag).Value.String()
+		cfg.GRPCPort = grpc
+	}
 
 	cfg.IP = coreIP
-	cfg.RPCPort = rpc
-	cfg.GRPCPort = grpc
 	return cfg.Validate()
 }

nodebuilder/core/flags_test.go

@@ -0,0 +1,154 @@
+package core
+
+import (
+	"testing"
+
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/require"
+)
+
+func TestParseFlags(t *testing.T) {
+	tests := []struct {
+		name        string
+		args        []string
+		inputCfg    Config // config that could be read from ctx
+		expectedCfg Config
+		expectError bool
+	}{
+		{
+			name:     "no flags",
+			args:     []string{},
+			inputCfg: Config{},
+			expectedCfg: Config{
+				IP:       "",
+				RPCPort:  "",
+				GRPCPort: "",
+			},
+			expectError: false,
+		},
+		{
+			name: "only core.ip",
+			args: []string{"--core.ip=127.0.0.1"},
+			inputCfg: Config{
+				RPCPort:  DefaultRPCPort,
+				GRPCPort: DefaultGRPCPort,
+			},
+			expectedCfg: Config{
+				IP:       "127.0.0.1",
+				RPCPort:  DefaultRPCPort,
+				GRPCPort: DefaultGRPCPort,
+			},
+			expectError: false,
+		},
+		{
+			name:     "only core.ip, empty port values",
+			args:     []string{"--core.ip=127.0.0.1"},
+			inputCfg: Config{},
+			expectedCfg: Config{
+				IP:       "127.0.0.1",
+				RPCPort:  DefaultRPCPort,
+				GRPCPort: DefaultGRPCPort,
+			},
+			expectError: false,
+		},
+		{
+			name: "no flags, values from input config.toml ",
+			args: []string{},
+			inputCfg: Config{
+				IP:       "127.162.36.1",
+				RPCPort:  "1234",
+				GRPCPort: "5678",
+			},
+			expectedCfg: Config{
+				IP:       "127.162.36.1",
+				RPCPort:  "1234",
+				GRPCPort: "5678",
+			},
+			expectError: false,
+		},
+		{
+			name: "only core.ip, with config.toml overridden defaults for ports",
+			args: []string{"--core.ip=127.0.0.1"},
+			inputCfg: Config{
+				RPCPort:  "1234",
+				GRPCPort: "5678",
+			},
+			expectedCfg: Config{
+				IP:       "127.0.0.1",
+				RPCPort:  "1234",
+				GRPCPort: "5678",
+			},
+			expectError: false,
+		},
+		{
+			name: "core.ip and core.rpc.port",
+			args: []string{"--core.ip=127.0.0.1", "--core.rpc.port=12345"},
+			inputCfg: Config{
+				RPCPort:  DefaultRPCPort,
+				GRPCPort: DefaultGRPCPort,
+			},
+			expectedCfg: Config{
+				IP:       "127.0.0.1",
+				RPCPort:  "12345",
+				GRPCPort: DefaultGRPCPort,
+			},
+			expectError: false,
+		},
+		{
+			name: "core.ip and core.grpc.port",
+			args: []string{"--core.ip=127.0.0.1", "--core.grpc.port=54321"},
+			inputCfg: Config{
+				RPCPort:  DefaultRPCPort,
+				GRPCPort: DefaultGRPCPort,
+			},
+			expectedCfg: Config{
+				IP:       "127.0.0.1",
+				RPCPort:  DefaultRPCPort,
+				GRPCPort: "54321",
+			},
+			expectError: false,
+		},
+		{
+			name: "core.ip, core.rpc.port, and core.grpc.port",
+			args: []string{"--core.ip=127.0.0.1", "--core.rpc.port=12345", "--core.grpc.port=54321"},
+			expectedCfg: Config{
+				IP:       "127.0.0.1",
+				RPCPort:  "12345",
+				GRPCPort: "54321",
+			},
+			expectError: false,
+		},
+		{
+			name:        "core.rpc.port without core.ip",
+			args:        []string{"--core.rpc.port=12345"},
+			expectedCfg: Config{},
+			expectError: true,
+		},
+		{
+			name:        "core.grpc.port without core.ip",
+			args:        []string{"--core.grpc.port=54321"},
+			expectedCfg: Config{},
+			expectError: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cmd := &cobra.Command{}
+			flags := Flags()
+			cmd.Flags().AddFlagSet(flags)
+			cmd.SetArgs(tt.args)
+
+			err := cmd.Execute()
+			require.NoError(t, err)
+
+			err = ParseFlags(cmd, &tt.inputCfg)
+			if tt.expectError {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, tt.expectedCfg, tt.inputCfg)
+			}
+		})
+	}
+}