Exit with non-0 status when encountering errors. Make usage errors less verbose. Other nits.

rocketpool-cli/rocketpool-cli.go

@@ -3,6 +3,7 @@ package main
 import (
 	"context"
 	"fmt"
+	"io"
 	"math/big"
 	"os"
 
@@ -218,12 +219,32 @@ func main() {
 		return nil
 	}
 
-	// Run application
-	fmt.Println("")
-	if err := app.Run(context.Background(), os.Args); err != nil {
-		cliutils.PrettyPrintError(err)
+	// Disable urfav's built-in usage error handling since it
+	//  1) prints full usage which is extremely verbose
+	//  2) doesn't cause an exit with non-0 code which is annoying
+	//  3) since we have to check for an error anyway to exit non-0, we may as well also print it
+	var usageError bool
+	app.OnUsageError = func(_ context.Context, cmd *cli.Command, err error, _ bool) error {
+		usageError = true
+
+		return err
 	}
 
-	fmt.Println("")
+	// Also disable the ErrWriter, since it is really annoying to get duplicate error messages, and we will print them manually
+	app.ErrWriter = io.Discard
+
+	// Run application and exit if it succeeds
+	err := app.Run(context.Background(), os.Args)
+	if err == nil {
 
+		return
+	}
+
+	// Print the error to stderr
+	fmt.Fprintf(os.Stderr, "%s\n", cliutils.PrettyError(err))
+	// Gently remind the operator how to check usage
+	if usageError {
+		fmt.Printf("Use `%s [subcommand] --help` to check usage.\n", os.Args[0])
+	}
+	os.Exit(1)
 }

rocketpool-cli/service/commands.go

@@ -22,7 +22,8 @@ func getComposeFiles(c *cli.Command) []string {
 }
 
 // Creates CLI argument flags from the parameters of the configuration struct
-func createFlagsFromConfigParams(sectionName string, params []*cfgtypes.Parameter, configFlags []cli.Flag, network cfgtypes.Network) []cli.Flag {
+func createFlagsFromConfigParams(sectionName string, params []*cfgtypes.Parameter, network cfgtypes.Network) []cli.Flag {
+	configFlags := make([]cli.Flag, 0)
 	for _, param := range params {
 		var paramName string
 		if sectionName == "" {
@@ -91,16 +92,15 @@ func createFlagsFromConfigParams(sectionName string, params []*cfgtypes.Paramete
 // Register commands
 func RegisterCommands(app *cli.Command, name string, aliases []string) {
 
-	configFlags := []cli.Flag{}
 	cfgTemplate := config.NewRocketPoolConfig("", false)
 	network := cfgTemplate.Smartnode.Network.Value.(cfgtypes.Network)
 
 	// Root params
-	configFlags = createFlagsFromConfigParams("", cfgTemplate.GetParameters(), configFlags, network)
+	configFlags := createFlagsFromConfigParams("", cfgTemplate.GetParameters(), network)
 
 	// Subconfigs
 	for sectionName, subconfig := range cfgTemplate.GetSubconfigs() {
-		configFlags = createFlagsFromConfigParams(sectionName, subconfig.GetParameters(), configFlags, network)
+		configFlags = append(configFlags, createFlagsFromConfigParams(sectionName, subconfig.GetParameters(), network)...)
 	}
 
 	app.Commands = append(app.Commands, &cli.Command{

rocketpool-cli/service/service.go

@@ -182,11 +182,7 @@ func serviceStatus(composeFiles []string) error {
 
 }
 
-func configureServicePrecheck() (isNew bool, cfg, oldCfg *config.RocketPoolConfig, err error) {
-
-	// Get RP client
-	rp := rocketpool.NewClient()
-	defer rp.Close()
+func configureServicePrecheck(rp *rocketpool.Client) (isNew bool, cfg, oldCfg *config.RocketPoolConfig, err error) {
 
 	// Load the config, checking to see if it's new (hasn't been installed before)
 	cfg, isNew, err = rp.LoadConfig()
@@ -210,7 +206,7 @@ func configureServicePrecheck() (isNew bool, cfg, oldCfg *config.RocketPoolConfi
 		}
 	}
 
-	cfg.ConfirmUpdateSuggestedSettings()
+	cfg.WarnUpdateSuggestedSettings()
 
 	return isNew, cfg, oldCfg, nil
 }
@@ -223,7 +219,7 @@ func configureServiceHeadless(c *cli.Command) error {
 	rp := rocketpool.NewClient()
 	defer rp.Close()
 
-	_, cfg, _, err := configureServicePrecheck()
+	_, cfg, _, err := configureServicePrecheck(rp)
 	if err != nil {
 		return err
 	}
@@ -246,7 +242,7 @@ func configureServiceHeadless(c *cli.Command) error {
 		}
 	}
 
-	return nil
+	return rp.SaveConfig(cfg)
 }
 
 // Configure the service
@@ -255,7 +251,7 @@ func configureService(configPath string, isNative, yes bool, composeFiles []stri
 	rp := rocketpool.NewClient()
 	defer rp.Close()
 
-	isNew, cfg, oldCfg, err := configureServicePrecheck()
+	isNew, cfg, oldCfg, err := configureServicePrecheck(rp)
 	if err != nil {
 		return err
 	}
@@ -450,6 +446,8 @@ func updateConfigParamFromCliArg(c *cli.Command, sectionName string, param *cfgt
 			if !found {
 				return fmt.Errorf("error setting value for %s: [%s] is not one of the valid options", paramName, selection)
 			}
+		default:
+			return fmt.Errorf("error setting value for %s: unknown type %T", paramName, param.Type)
 		}
 	}
 

shared/services/config/rocket-pool-config.go

@@ -22,7 +22,6 @@ import (
 	"github.com/rocket-pool/smartnode/shared/services/config/migration"
 	addontypes "github.com/rocket-pool/smartnode/shared/types/addons"
 	"github.com/rocket-pool/smartnode/shared/types/config"
-	"github.com/rocket-pool/smartnode/shared/utils/cli/prompt"
 )
 
 // Constants
@@ -1546,34 +1545,32 @@ func (cfg *RocketPoolConfig) GetConfigTitle() string {
 	return cfg.Title
 }
 
-func (cfg *RocketPoolConfig) ConfirmUpdateSuggestedSettings() {
+func (cfg *RocketPoolConfig) WarnUpdateSuggestedSettings() {
+	gasLimitSettings := map[string]*config.Parameter{
+		"consensus": &cfg.ConsensusCommon.SuggestedBlockGasLimit,
+		"execution": &cfg.ExecutionCommon.SuggestedBlockGasLimit,
+	}
 
-	// If using the old consensus block gas limit, ask the user if they want to update it
-	if cfg.ConsensusCommon.SuggestedBlockGasLimit.Value != "" {
-		blockGasLimit, err := strconv.Atoi(cfg.ConsensusCommon.SuggestedBlockGasLimit.Value.(string))
-		if err != nil {
-			cfg.ConsensusCommon.SuggestedBlockGasLimit.Value = ""
-		}
-		if blockGasLimit < coreDevsSuggestedGasLimit {
-			if prompt.Confirm("Your consensus block gas limit setting is currently '%d' . The maintainers suggest changing it to use the updated consensus client value. Would you like to update your setting?", blockGasLimit) {
-				cfg.ConsensusCommon.SuggestedBlockGasLimit.Value = ""
-			}
+	for name, target := range gasLimitSettings {
+		if target.Value == "" {
+			continue
 		}
-	}
 
-	// If using the old execution block gas limit, ask the user if they want to update it
-	if cfg.ExecutionCommon.SuggestedBlockGasLimit.Value != "" {
-		blockGasLimit, err := strconv.Atoi(cfg.ExecutionCommon.SuggestedBlockGasLimit.Value.(string))
+		blockGasLimit, err := strconv.Atoi(target.Value.(string))
 		if err != nil {
-			cfg.ExecutionCommon.SuggestedBlockGasLimit.Value = ""
+			target.Value = ""
+			continue
 		}
 		if blockGasLimit < coreDevsSuggestedGasLimit {
-			if prompt.Confirm("Your execution block gas limit setting is currently '%d' . The maintainers suggest changing it to use the updated consensus client value. Would you like to update your setting?", blockGasLimit) {
-				cfg.ExecutionCommon.SuggestedBlockGasLimit.Value = ""
-			}
+			fmt.Fprintf(
+				os.Stderr,
+				"Warning: Your %s block gas limit setting is currently '%d'. The maintainers suggest changing it to use the updated consensus client value '%d'\n",
+				name,
+				blockGasLimit,
+				coreDevsSuggestedGasLimit,
+			)
 		}
 	}
-
 }
 
 // Update the default settings for all overwrite-on-upgrade parameters

shared/utils/cli/utils.go

@@ -85,36 +85,24 @@ func GetDateTimeString(dateTime uint64) string {
 
 // Gets the hex string of an address, or "none" if it was the 0x0 address
 func GetPrettyAddress(address common.Address) string {
-	addressString := address.Hex()
-	if addressString == "0x0000000000000000000000000000000000000000" {
+	if address.Cmp(common.Address{}) == 0 {
 		return "<none>"
 	}
-	return addressString
-}
-
-// Temporary table for replacing revert messages with more useful versions until we can refactor
-var errorMap = map[string]string{
-	"Could not get can node deposit status: Minipool count after deposit exceeds limit based on node RPL stake": "Cannot create a new minipool: you do not have enough RPL staked to create another minipool.",
+	return address.Hex()
 }
 
 // Prints an error in a prettier format, removing the "stack trace" if it represents
 // a contract revert message
-func PrettyPrintError(err error) {
-	errorMessage := err.Error()
-	prettyErr := errorMessage
-	if strings.Contains(errorMessage, "execution reverted:") {
-		elements := strings.Split(errorMessage, ":")
+func PrettyError(err error) string {
+	prettyErr := err.Error()
+	if strings.Contains(prettyErr, "execution reverted:") {
+		elements := strings.Split(prettyErr, ":")
 		firstMessage := strings.TrimSpace(elements[0])
-		secondMessage := strings.TrimSpace(elements[len(elements)-1])
-		prettyErr = fmt.Sprintf("%s: %s", firstMessage, secondMessage)
-
-		// Look for the message in the above error table and replace if appropriate
-		replacementMessage, exists := errorMap[prettyErr]
-		if exists {
-			prettyErr = replacementMessage
-		}
+		// TODO: Determine if it's safer to join elements[1:]
+		lastMessage := strings.TrimSpace(elements[len(elements)-1])
+		prettyErr = fmt.Sprintf("%s: %s", firstMessage, lastMessage)
 	}
-	fmt.Println(prettyErr)
+	return prettyErr
 }
 
 // Prints an error message when the Beacon client is not using the deposit contract address that Rocket Pool expects