feat(k3d): refactor suggestion

[FalcoSuessgott]

Disclaimer:
I was playing around with kubefirst and k3d on GitHub and was checking out the code, since I'm currently traveling and looking for some OSS projects to contribute. The PR turned out to be way bigger than anticipated... I understand in case you guys don't want to invest the time in reviewing. I could also split up and open up multiple PRs containing the changes described next, in case some changes are of interest. I am also willing to expand these changes to the other supported cloud provider.

I want to suggest some alternative approaches when building CLI apps with cobra, I choose k3d as an example, since I am able to fully e2e test it locally.

The changes can be summarized into:

prechecks

the k3d create now has a precheck suite, where certain assertions can be made (env var exists, command is available, ...) and eventually error and thus inform the user early on about missing dependencies, before creating any resources. This can be easily tested and expanded for other prechecks.

		PreRunE: func(cmd *cobra.Command, args []string) error {
			// example implementation of a pre check suite where we could validate & verify various required settings (Github access, env vars, commands, ...)
			fmt.Println("[PRECHECKS] Running prechecks")

			// invalid git provider
			if !utilities.StringInSlice(opts.gitProvider, supportedGitProviders) {
				return fmt.Errorf("\"%s\" is not a supported git provider", opts.gitProvider)
			}

			// invalid git protocol
			if !utilities.StringInSlice(opts.gitProtocol, supportedGitProtocolOverride) {
				return fmt.Errorf("%s is not a support git protocol", opts.gitProtocol)
			}

			// github specific prechecks
			if strings.ToLower(opts.gitProvider) == "github" {
				// enforce for GITHUB_TOKEN
				if !prechecks.EnvVarExists("GITHUB_TOKEN") {
					return fmt.Errorf("GITHUB_TOKEN not set, but required when using GitHub")
				}

				// github.com is available
				if err := prechecks.URLIsAvailable("https://github.com"); err != nil {
					return fmt.Errorf("github.com is not available, try again")
				}

				// check for known hosts of github.com if git provider is github
				if err := prechecks.CheckKnownHosts("github.com"); err != nil && strings.ToLower(opts.gitProvider) == "github" {
					return err
				}
			}

			// gitlab specific prechecks
			if strings.ToLower(opts.gitProvider) == "gitlab" {
				// enforce GITLAB_TOKEN if provider is gitlab
				if !prechecks.EnvVarExists("GITLAB_TOKEN") {
					return fmt.Errorf("GITLAB_TOKEN not set, but required when using GitLab")
				}

				// gitlab.com is available
				if err := prechecks.URLIsAvailable("https://gitlab.com"); err != nil {
					return fmt.Errorf("gitlab.com is not available, try again")
				}

				// when gitlab, check for a specified group
				if opts.gitlabGroup == "" {
					return fmt.Errorf("a gitlab-group is required when using Gitlab")
				}

				// check known hosts for gitlab
				if err := prechecks.CheckKnownHosts("gitlab.com"); err != nil && strings.ToLower(opts.gitProvider) == "gitlab" {
					return err
				}
			}

			// enforce NGROK_AUTH_TOKEN
			if !prechecks.EnvVarExists("NGROK_AUTHTOKEN") {
				return fmt.Errorf("NGROK_AUTHTOKEN not set, but required")
			}

			// docker is installed
			if err := prechecks.CommandExists("docker"); err != nil {
				return fmt.Errorf("docker is not installed, but is required when using k3d")
			}

			// check docker is running
			if err := prechecks.CheckDockerIsRunning(); err != nil {
				return fmt.Errorf("docker is not running, but is required when using k3d")
			}

			// portforwarding ports are available
			if err := k8s.CheckForExistingPortForwards(portForwardingPorts...); err != nil {
				return fmt.Errorf("%s - this port is required to set up your kubefirst environment - please close any existing port forwards before continuing", err.Error())
			}

			// verify user has specified valid catalog apps to be installed
			isValid, apps, err := catalog.ValidateCatalogApps(opts.installCatalogApps)
			if !isValid || err != nil {
				return err
			}

			opts.catalogApps = apps

			// check available disk space
			if err := prechecks.CheckAvailableDiskSize(); err != nil {
				return fmt.Errorf("error checking available disk size: %s", err.Error())
			}

			fmt.Println("[PRECHECKS] all prechecks passed - continuing with k3d cluster bootstrapping")

			return nil
		},

decluttered the main function

I was able to declutter the main function to more idomatic way and moved the bubbletea and logging initialiaztion in cobra.OnInitialize(), as well as the bubbletea quitting in the PostRun():

package main

import (
	"fmt"
	"os"

	"github.com/kubefirst/kubefirst/cmd"
)

func main() {
	if err := cmd.Execute(); err != nil {
		fmt.Fprintf(os.Stderr, "[ERROR] %v.\n", err)
		fmt.Fprintf(os.Stderr, "\nIf a detailed error message was available, please make the necessary corrections before retrying.\nYou can re-run the last command to try the operation again.\n\n")

		os.Exit(1)
	}
}

split up the k3d create logic in functions

The biggest change might be the split up of the runK3dCreate function into functions. Where possible, I avoided if/else for more readability. To do so, I added a struct that contains all required options and sane defaults for creating the k3d cluster. I believe this way the code is more readable and most important testable, since I haven't found any tests so far.. I think some refactors in the underlying libs (kubefirst/runtime, kubefrist/kubefirst-api etc) might be worth it

type createOptions struct {
	ci bool

	cloudRegion string
	clusterName string
	clusterType string
	clusterId   string

	containerRegistryHost string

	githubUser string
	githubOrg  string

	gitlabGroup   string
	gitlabGroupId int

	gitProvider   string
	gitProtocol   string
	gitToken      string
	gitHost       string
	gitUser       string
	gitOwner      string
	gitDescriptor string

	gitopsTemplateURL    string
	gitopsTemplateBranch string
	gitopsRepoURL        string

	useTelemetry bool
	segClient    telemetry.TelemetryEvent

	httpClient *http.Client

	installCatalogApps string
	catalogApps        []types.GitopsCatalogApp
}

func defaultCreateOpts() *createOptions {
	return &createOptions{
		clusterName: "kubefirst",
		clusterType: "mgmt",

		gitProvider: "github",
		gitProtocol: "ssh",

		gitopsTemplateURL:    "https://github.com/kubefirst/gitops-template.git",
		gitopsTemplateBranch: "main",

		useTelemetry: true,

		httpClient: http.DefaultClient,

		segClient: telemetry.TelemetryEvent{},
	}
}

This allows us to eadily call functions based on the current gitprovider:

// github specific auth settings
if strings.ToLower(o.gitProvider) == "github" {
	if err := o.githubAuth(); err != nil {
		return fmt.Errorf("error during github auth: %s", err.Error())
	}
}

func (o *createOptions) githubAuth() error {
	o.gitHost = k3d.GithubHost
	o.containerRegistryHost = "ghcr.io"

	// Attempt to retrieve session-scoped token for GitHub user
	gitHubService := services.NewGitHubService(o.httpClient)
	gitHubHandler := handlers.NewGitHubHandler(gitHubService)

	ghToken := utilities.EnvOrDefault("GITHUB_TOKEN", viper.GetString("github.session_token"))
	gitHubAccessToken, err := wrappers.AuthenticateGitHubUserWrapper(ghToken, gitHubHandler)
	if err != nil {
		log.Warn().Msgf(err.Error())
	}

	o.gitToken = gitHubAccessToken

	if err := github.VerifyTokenPermissions(o.gitToken); err != nil {
		return err
	}
        ....
}

other smaller changes

• check error when saving viper config
• check error of telemetry events
• use cobra flags constraints for marking flags required or mutually exclusive
• correctly disable telemetry when specifying via flag
• added TF_CLI_ARGS="-no-color" to tf apply runs for having no color encodings in the logfile
• added some simple tests showing how to test the refactored kubefirst k3d command by passing custom flags and envs

[fharper]

@FalcoSuessgott the PR is still in draft, but do you want us to review and validate the refactoring idea now?

[FalcoSuessgott]

HI @fharper, yes its ready for review, I added some simple tests illustrating the purpose of some changes

[fharper]

@FalcoSuessgott: sorry for the delay, I asked the engineering team again to give it a closer look as soon as possible.

[FalcoSuessgott]

Merged already😳 I hoped someone tested this and was able to verify the changes :D Nevertheless Im available and can fix if something shows up

[fharper]

@FalcoSuessgott oh weird, my mistake, not sure what happened as I just wanted to merge main into your main branch to be sure people have the latest when reviewing it. I'll remove the merge on main as nobody from the engineering team had the time to check it yet, but I pinged them again this morning! Thanks for noticing...

[fharper]

Oh no, it's fine, nothing was merged in kubefirst main. GitHub wasn't clear on how it wrote that, but I merged kubefirst main in your fork main... You can see there are still 51 files changed when looking at this PR, and nothing about this is in kubefirst main branch logs.

As a side note, it's part of why I always suggest that folks create a branch in their fork before doing a PR, so the verbose and actions are clearer in the GitHub interface (will add this to the contributions guidelines) :)

[fharper]

@FalcoSuessgott I'm so sorry about the delay, we usually respond a lot faster to PR. @jarededwards told me he will review it as soon as possible.