From e8258edc8e2287c7b5e528dde6ce4da8e6493c7d Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 6 Aug 2026 15:12:50 +0000 Subject: [PATCH 1/5] fix(lint): resolve funlen findings in cobra command factories Group A of the funlen backlog: cobra command factories and flag registration functions were long because they registered many flags in a flat sequence. Split each into a small registerFlags dispatcher calling logically-grouped registerFlags helpers, following the existing convention in cmd/workspace/up/up_flags.go (registerFlags -> registerSSHFlags, registerDotfilesFlags, etc.). - cmd/ci/ci.go: registerFlags -> registerRunFlags/registerBuildFlags/ registerWorkspaceFlags/registerSecretsFlags - cmd/flags/flags.go: SetGlobalFlags -> registerCoreFlags/registerOutputFlags/ registerVerbosityFlags/registerHiddenFlags/bindGlobalEnvVars - cmd/internal/agentcontainer/setup.go: NewSetupContainerCmd -> registerFlags dispatching to registerBehaviorFlags/registerWorkspaceInfoFlags/ registerDotfilesFlags - cmd/internal/runusercommands.go: NewRunUserCommandsCmd -> registerFlags dispatching to registerTargetFlags/registerConfigFlags/registerEnvFlags/ registerLifecycleFlags - cmd/pro/cluster/add.go: NewAddCmd's flag registration moved to new cmd/pro/cluster/add_flags.go (registerFlags -> registerIdentityFlags/ registerBehaviorFlags/registerHelmFlags/registerClusterFlags) - cmd/pro/start.go: NewStartCmd's flag registration moved to new cmd/pro/start_flags.go (registerFlags -> registerDockerFlags/ registerClusterFlags/registerChartFlags/registerAuthFlags/ registerLifecycleFlags) - cmd/provider/init.go: NewInitCmd's flag registration moved to new cmd/provider/init_flags.go (registerFlags -> registerOptionFlags/ registerTestingFlags) - cmd/workspace/ssh.go: NewSSHCmd -> registerFlags dispatching to registerPortForwardingFlags/registerEnvFlags/registerSessionFlags/ registerAgentForwardingFlags/registerServiceFlags/registerTerminalFlags - cmd/workspace/up/up_flags.go: registerWorkspaceFlags itself split further into registerWorkspaceIdentityFlags/registerWorkspaceSecretsFlags/ registerWorkspaceRuntimeFlags Every flag name, default, shorthand, hidden marker, and env binding is preserved verbatim; only the grouping changed. --- cmd/ci/ci.go | 36 ++++- cmd/flags/flags.go | 36 +++-- cmd/internal/agentcontainer/setup.go | 161 ++++++++++++-------- cmd/internal/runusercommands.go | 37 ++++- cmd/pro/cluster/add.go | 63 +------- cmd/pro/cluster/add_flags.go | 96 ++++++++++++ cmd/pro/start.go | 215 ++++++++++----------------- cmd/pro/start_flags.go | 137 +++++++++++++++++ cmd/provider/init.go | 13 +- cmd/provider/init_flags.go | 30 ++++ cmd/workspace/ssh.go | 124 ++++++++++++--- cmd/workspace/up/up_flags.go | 18 +++ 12 files changed, 652 insertions(+), 314 deletions(-) create mode 100644 cmd/pro/cluster/add_flags.go create mode 100644 cmd/pro/start_flags.go create mode 100644 cmd/provider/init_flags.go diff --git a/cmd/ci/ci.go b/cmd/ci/ci.go index afcf8d518..4c5c55949 100644 --- a/cmd/ci/ci.go +++ b/cmd/ci/ci.go @@ -67,6 +67,13 @@ exit code of "devsy ci".`, } func (cmd *CICmd) registerFlags(ciCmd *cobra.Command) { + cmd.registerRunFlags(ciCmd) + cmd.registerBuildFlags(ciCmd) + cmd.registerWorkspaceFlags(ciCmd) + cmd.registerSecretsFlags(ciCmd) +} + +func (cmd *CICmd) registerRunFlags(ciCmd *cobra.Command) { cliflags.Add(ciCmd, cliflags.String(&cmd.RunCmdString, runCmdFlag, "", "Shell command to run inside the container, executed via sh -c "+ @@ -75,14 +82,15 @@ func (cmd *CICmd) registerFlags(ciCmd *cobra.Command) { "Environment variables to set in the container at run time (KEY=VALUE, repeatable)"), cliflags.Bool(&cmd.Keep, keepFlag, false, "Keep the workspace after running instead of tearing it down (useful for debugging)"), + ) +} + +func (cmd *CICmd) registerBuildFlags(ciCmd *cobra.Command) { + cliflags.Add(ciCmd, cliflags.String(&cmd.DevContainerSource, names.DevContainer, "", "Select the devcontainer config source, overriding project discovery: "+ `"none" (ignore the project config), "image:" (use only that image), `+ `"id:" (a named .devcontainer/ profile), or a path to a devcontainer.json`), - cliflags.StringSlice(&cmd.ProviderOptions, names.ProviderOption, nil, - "Provider option in the form KEY=VALUE"), - cliflags.String(&cmd.Machine, names.Machine, "", - "The machine to use for this workspace. The machine needs to exist beforehand"), cliflags.Bool(&cmd.NoCache, names.NoCache, false, "Do not use the build cache when building the image"), cliflags.String(&cmd.RunPlatform, names.Platform, "", @@ -95,6 +103,18 @@ func (cmd *CICmd) registerFlags(ciCmd *cobra.Command) { cliflags.StringArray(&cmd.CacheFrom, names.CacheFrom, nil, "Cache sources for the build (e.g., myregistry.io/cache:latest). "+ "Reuse a pre-built image to warm the build"), + ) + cliflags.RegisterDevContainerModifierFlags(ciCmd.Flags(), cliflags.DevContainerModifierFlags{ + Features: &cmd.AdditionalFeatures, + }) +} + +func (cmd *CICmd) registerWorkspaceFlags(ciCmd *cobra.Command) { + cliflags.Add(ciCmd, + cliflags.StringSlice(&cmd.ProviderOptions, names.ProviderOption, nil, + "Provider option in the form KEY=VALUE"), + cliflags.String(&cmd.Machine, names.Machine, "", + "The machine to use for this workspace. The machine needs to exist beforehand"), cliflags.StringArray(&cmd.WorkspaceEnv, names.WorkspaceEnv, nil, "Env variables available at build/lifecycle time (KEY=VALUE, repeatable)"), cliflags.StringSlice(&cmd.WorkspaceEnvFile, names.WorkspaceEnvFile, nil, @@ -105,6 +125,11 @@ func (cmd *CICmd) registerFlags(ciCmd *cobra.Command) { nil, "Extra env variables to inject during workspace initialization (KEY=VALUE, repeatable)", ), + ) +} + +func (cmd *CICmd) registerSecretsFlags(ciCmd *cobra.Command) { + cliflags.Add(ciCmd, cliflags.String( &cmd.SecretsFile, names.SecretsFile, @@ -127,9 +152,6 @@ func (cmd *CICmd) registerFlags(ciCmd *cobra.Command) { cliflags.String(&cmd.GitTokenUsername, names.GitTokenUsername, "", "Username for --git-token (default inferred from the repo host)"), ) - cliflags.RegisterDevContainerModifierFlags(ciCmd.Flags(), cliflags.DevContainerModifierFlags{ - Features: &cmd.AdditionalFeatures, - }) } func splitArgs( diff --git a/cmd/flags/flags.go b/cmd/flags/flags.go index bb6280133..a8e8284ec 100644 --- a/cmd/flags/flags.go +++ b/cmd/flags/flags.go @@ -26,12 +26,32 @@ type GlobalFlags struct { func SetGlobalFlags(flags *flag.FlagSet) *GlobalFlags { globalFlags := &GlobalFlags{} + registerCoreFlags(flags, globalFlags) + registerOutputFlags(flags, globalFlags) + registerVerbosityFlags(flags, globalFlags) + registerHiddenFlags(flags, globalFlags) + bindGlobalEnvVars(flags) + + return globalFlags +} + +func registerCoreFlags(flags *flag.FlagSet, globalFlags *GlobalFlags) { flags.StringVar( &globalFlags.DevsyHome, names.Home, "", "If defined will override the default devsy home", ) + flags.StringVar(&globalFlags.Context, names.Context, "", "The context to use") + flags.StringVar( + &globalFlags.Provider, + names.Provider, + "", + "The provider to use. Needs to be configured for the selected context", + ) +} + +func registerOutputFlags(flags *flag.FlagSet, globalFlags *GlobalFlags) { flags.StringVar( &globalFlags.ResultFormat, names.ResultFormat, @@ -46,13 +66,9 @@ func SetGlobalFlags(flags *flag.FlagSet) *GlobalFlags { ) flags.StringVar(&globalFlags.LogOutput, names.LogFormat, "text", "Alias for --log-output") _ = flags.MarkHidden(names.LogFormat) - flags.StringVar(&globalFlags.Context, names.Context, "", "The context to use") - flags.StringVar( - &globalFlags.Provider, - names.Provider, - "", - "The provider to use. Needs to be configured for the selected context", - ) +} + +func registerVerbosityFlags(flags *flag.FlagSet, globalFlags *GlobalFlags) { flags.CountVarP( &globalFlags.Verbosity, names.Verbose, @@ -72,7 +88,9 @@ func SetGlobalFlags(flags *flag.FlagSet) *GlobalFlags { false, "Enable debug logging (equivalent to -vv)", ) +} +func registerHiddenFlags(flags *flag.FlagSet, globalFlags *GlobalFlags) { flags.Var(&globalFlags.Owner, names.Owner, "Show pro workspaces for owner") _ = flags.MarkHidden(names.Owner) flags.StringVar(&globalFlags.UID, names.UID, "", "Set UID for workspace") @@ -84,11 +102,11 @@ func SetGlobalFlags(flags *flag.FlagSet) *GlobalFlags { "The data folder where agent data is stored.", ) _ = flags.MarkHidden(names.AgentDir) +} +func bindGlobalEnvVars(flags *flag.FlagSet) { pkgflags.BindEnv(flags, names.Home) pkgflags.BindEnv(flags, names.Context) pkgflags.BindEnv(flags, names.Provider) pkgflags.BindEnv(flags, names.Debug) - - return globalFlags } diff --git a/cmd/internal/agentcontainer/setup.go b/cmd/internal/agentcontainer/setup.go index c0db3a416..75c59db61 100644 --- a/cmd/internal/agentcontainer/setup.go +++ b/cmd/internal/agentcontainer/setup.go @@ -76,6 +76,18 @@ func NewSetupContainerCmd(globalFlags *flags.GlobalFlags) *cobra.Command { return cmd.Run(cobraCmd.Context()) }, } + cmd.registerFlags(setupContainerCmd) + + return setupContainerCmd +} + +func (cmd *SetupContainerCmd) registerFlags(setupContainerCmd *cobra.Command) { + cmd.registerBehaviorFlags(setupContainerCmd) + cmd.registerWorkspaceInfoFlags(setupContainerCmd) + cmd.registerDotfilesFlags(setupContainerCmd) +} + +func (cmd *SetupContainerCmd) registerBehaviorFlags(setupContainerCmd *cobra.Command) { cliflags.Add( setupContainerCmd, cliflags.Bool( @@ -102,6 +114,12 @@ func NewSetupContainerCmd(globalFlags *flags.GlobalFlags) *cobra.Command { false, "If Devsy should inject git credentials during setup", ), + ) +} + +func (cmd *SetupContainerCmd) registerWorkspaceInfoFlags(setupContainerCmd *cobra.Command) { + cliflags.Add( + setupContainerCmd, cliflags.String( &cmd.ContainerWorkspaceInfo, names.ContainerWorkspaceInfo, @@ -112,6 +130,16 @@ func NewSetupContainerCmd(globalFlags *flags.GlobalFlags) *cobra.Command { cliflags.String(&cmd.AccessKey, names.AccessKey, "", "Access Key to use"), cliflags.String(&cmd.WorkspaceHost, names.WorkspaceHost, "", "Workspace hostname to use"), cliflags.String(&cmd.PlatformHost, names.PlatformHost, "", "Platform host"), + ) + _ = setupContainerCmd.MarkFlagRequired(names.SetupInfo) + + cliflags.BindEnv(setupContainerCmd.Flags(), names.AccessKey) + cliflags.BindEnv(setupContainerCmd.Flags(), names.PlatformHost) +} + +func (cmd *SetupContainerCmd) registerDotfilesFlags(setupContainerCmd *cobra.Command) { + cliflags.Add( + setupContainerCmd, cliflags.String(&cmd.DotfilesRepo, names.DotfilesRepo, "", "Dotfiles repository URL"), cliflags.String( &cmd.DotfilesScript, @@ -120,12 +148,6 @@ func NewSetupContainerCmd(globalFlags *flags.GlobalFlags) *cobra.Command { "Dotfiles install script path", ), ) - _ = setupContainerCmd.MarkFlagRequired(names.SetupInfo) - - cliflags.BindEnv(setupContainerCmd.Flags(), names.AccessKey) - cliflags.BindEnv(setupContainerCmd.Flags(), names.PlatformHost) - - return setupContainerCmd } type setupContext struct { @@ -919,74 +941,95 @@ func streamMount( ) error { // if we have a platform workspace socket we connect directly to it if workspaceInfo.CLIOptions.Platform.Enabled { - // check if the runner proxy socket exists - httpClient := &http.Client{ - Transport: &http.Transport{ - TLSClientConfig: &tls.Config{ - InsecureSkipVerify: true, - }, + return streamMountFromPlatform(ctx, workspaceInfo, m) + } + + return streamMountFromTunnel(ctx, m, tunnelClient) +} + +func streamMountFromPlatform( + ctx context.Context, + workspaceInfo *provider2.ContainerWorkspaceInfo, + m *config.Mount, +) error { + log.Infof("Download %s into DevContainer %s", m.Source, m.Target) + req, err := buildPlatformDownloadRequest(ctx, workspaceInfo, m) + if err != nil { + return err + } + + httpClient := &http.Client{ + Transport: &http.Transport{ + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, }, - } + }, + } + resp, err := httpClient.Do(req) + if err != nil { + return fmt.Errorf("download workspace: %w", err) + } + defer func() { _ = resp.Body.Close() }() - // build the url - log.Infof("Download %s into DevContainer %s", m.Source, m.Target) - url := fmt.Sprintf( - "https://%s/kubernetes/management/apis/management.devsy.sh/v1/namespaces/%s/devsyworkspaceinstances/%s/download?path=%s", - ts.RemoveProtocol(workspaceInfo.CLIOptions.Platform.PlatformHost), - workspaceInfo.CLIOptions.Platform.InstanceNamespace, - workspaceInfo.CLIOptions.Platform.InstanceName, - url.QueryEscape(m.Source), - ) - req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) - if err != nil { - return fmt.Errorf("create request: %w", err) - } - req.Header.Set( - "Authorization", - fmt.Sprintf("Bearer %s", workspaceInfo.CLIOptions.Platform.AccessKey), + // check if the response is ok + if resp.StatusCode != http.StatusOK { + body, _ := io.ReadAll(resp.Body) + return fmt.Errorf( + "download workspace: body = %s, status = %s", + string(body), + resp.Status, ) + } - // send the request - resp, err := httpClient.Do(req) - if err != nil { - return fmt.Errorf("download workspace: %w", err) - } - defer func() { _ = resp.Body.Close() }() - - // check if the response is ok - if resp.StatusCode != http.StatusOK { - body, _ := io.ReadAll(resp.Body) - return fmt.Errorf( - "download workspace: body = %s, status = %s", - string(body), - resp.Status, - ) - } + progressReader := &progressReader{ + Reader: resp.Body, + } - // create progress reader - progressReader := &progressReader{ - Reader: resp.Body, - } + if err := extract.Extract(progressReader, m.Target); err != nil { + return fmt.Errorf("stream mount %s: %w", m.String(), err) + } - // target folder - err = extract.Extract(progressReader, m.Target) - if err != nil { - return fmt.Errorf("stream mount %s: %w", m.String(), err) - } + return nil +} - return nil +// buildPlatformDownloadRequest builds the authenticated request to the +// runner proxy socket that serves m.Source for download. +func buildPlatformDownloadRequest( + ctx context.Context, + workspaceInfo *provider2.ContainerWorkspaceInfo, + m *config.Mount, +) (*http.Request, error) { + downloadURL := fmt.Sprintf( + "https://%s/kubernetes/management/apis/management.devsy.sh/v1/namespaces/%s/devsyworkspaceinstances/%s/download?path=%s", + ts.RemoveProtocol(workspaceInfo.CLIOptions.Platform.PlatformHost), + workspaceInfo.CLIOptions.Platform.InstanceNamespace, + workspaceInfo.CLIOptions.Platform.InstanceName, + url.QueryEscape(m.Source), + ) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, downloadURL, nil) + if err != nil { + return nil, fmt.Errorf("create request: %w", err) } + req.Header.Set( + "Authorization", + fmt.Sprintf("Bearer %s", workspaceInfo.CLIOptions.Platform.AccessKey), + ) - // stream mount + return req, nil +} + +func streamMountFromTunnel( + ctx context.Context, + m *config.Mount, + tunnelClient tunnel.TunnelClient, +) error { log.Infof("Copy %s into DevContainer %s", m.Source, m.Target) stream, err := tunnelClient.StreamMount(ctx, &tunnel.StreamMountRequest{Mount: m.String()}) if err != nil { return fmt.Errorf("init stream mount %s: %w", m.String(), err) } - // target folder - err = extract.Extract(tunnelserver.NewStreamReader(stream), m.Target) - if err != nil { + if err := extract.Extract(tunnelserver.NewStreamReader(stream), m.Target); err != nil { return fmt.Errorf("stream mount %s: %w", m.String(), err) } diff --git a/cmd/internal/runusercommands.go b/cmd/internal/runusercommands.go index 71608665e..516b0e9ab 100644 --- a/cmd/internal/runusercommands.go +++ b/cmd/internal/runusercommands.go @@ -54,6 +54,21 @@ func NewRunUserCommandsCmd(f *flags.GlobalFlags) *cobra.Command { RunE: runE, } + cmd.registerFlags(runCmd) + + runCmd.MarkFlagsOneRequired(names.WorkspaceFolder, names.ContainerID) + + return runCmd +} + +func (cmd *RunUserCommandsCmd) registerFlags(runCmd *cobra.Command) { + cmd.registerTargetFlags(runCmd) + cmd.registerConfigFlags(runCmd) + cmd.registerEnvFlags(runCmd) + cmd.registerLifecycleFlags(runCmd) +} + +func (cmd *RunUserCommandsCmd) registerTargetFlags(runCmd *cobra.Command) { cliflags.Add( runCmd, cliflags.String( @@ -74,6 +89,12 @@ func NewRunUserCommandsCmd(f *flags.GlobalFlags) *cobra.Command { "", "Path to the docker/podman executable (defaults to 'docker')", ), + ) +} + +func (cmd *RunUserCommandsCmd) registerConfigFlags(runCmd *cobra.Command) { + cliflags.Add( + runCmd, cliflags.String( &cmd.Config, names.Config, @@ -86,6 +107,12 @@ func NewRunUserCommandsCmd(f *flags.GlobalFlags) *cobra.Command { "", "Path to an additional devcontainer.json file to override the primary configuration", ), + ) +} + +func (cmd *RunUserCommandsCmd) registerEnvFlags(runCmd *cobra.Command) { + cliflags.Add( + runCmd, cliflags.StringArray( &cmd.RemoteEnv, names.RemoteEnv, @@ -98,6 +125,12 @@ func NewRunUserCommandsCmd(f *flags.GlobalFlags) *cobra.Command { []string{}, "Override the default container identification labels (format: key=value, can be specified multiple times)", ), + ) +} + +func (cmd *RunUserCommandsCmd) registerLifecycleFlags(runCmd *cobra.Command) { + cliflags.Add( + runCmd, cliflags.Bool( &cmd.Prebuild, names.Prebuild, @@ -136,10 +169,6 @@ func NewRunUserCommandsCmd(f *flags.GlobalFlags) *cobra.Command { "Skip running updateContentCommand", ), ) - - runCmd.MarkFlagsOneRequired(names.WorkspaceFolder, names.ContainerID) - - return runCmd } // NewRunUserCommandsCmdAlias creates the hidden camelCase alias for devcontainer CLI compat. diff --git a/cmd/pro/cluster/add.go b/cmd/pro/cluster/add.go index 85d1ef240..8d758dfd3 100644 --- a/cmd/pro/cluster/add.go +++ b/cmd/pro/cluster/add.go @@ -13,8 +13,6 @@ import ( storagev1 "github.com/devsy-org/api/pkg/apis/storage/v1" proflags "github.com/devsy-org/devsy/cmd/pro/flags" "github.com/devsy-org/devsy/pkg/config" - cliflags "github.com/devsy-org/devsy/pkg/flags" - "github.com/devsy-org/devsy/pkg/flags/names" "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/platform" "github.com/devsy-org/devsy/pkg/platform/client" @@ -61,66 +59,7 @@ func NewAddCmd(globalFlags *proflags.GlobalFlags) *cobra.Command { }, } - cliflags.Add( - c, - cliflags.String( - &cmd.Namespace, - names.Namespace, - "loft", - "The namespace to generate the service account in. The namespace will be created if it does not exist", - ), - cliflags.String( - &cmd.ServiceAccount, - names.ServiceAccount, - "loft-admin", - "The service account name to create", - ), - cliflags.String( - &cmd.DisplayName, - names.DisplayName, - "", - "The display name to show in the UI for this cluster", - ), - cliflags.Bool( - &cmd.Wait, - names.Wait, - false, - "If true, will wait until the cluster is initialized", - ), - cliflags.Bool( - &cmd.Insecure, - names.Insecure, - false, - "If true, deploys the agent in insecure mode", - ), - cliflags.String( - &cmd.HelmChartVersion, - names.HelmChartVersion, - "", - "The agent chart version to deploy", - ), - cliflags.String(&cmd.HelmChartPath, names.HelmChartPath, "", "The agent chart to deploy"), - cliflags.StringArray( - &cmd.HelmSet, - names.HelmSet, - []string{}, - "Extra helm values for the agent chart", - ), - cliflags.StringArray( - &cmd.HelmValues, - names.HelmValues, - []string{}, - "Extra helm values for the agent chart", - ), - cliflags.String( - &cmd.KubeContext, - names.KubeContext, - "", - "The kube context to use for installation", - ), - cliflags.String(&cmd.Host, names.Host, "", "The pro instance to use"), - ) - proflags.BindEnv(c.Flags(), names.Host) + cmd.registerFlags(c) return c } diff --git a/cmd/pro/cluster/add_flags.go b/cmd/pro/cluster/add_flags.go new file mode 100644 index 000000000..11decaadf --- /dev/null +++ b/cmd/pro/cluster/add_flags.go @@ -0,0 +1,96 @@ +package cluster + +import ( + proflags "github.com/devsy-org/devsy/cmd/pro/flags" + cliflags "github.com/devsy-org/devsy/pkg/flags" + "github.com/devsy-org/devsy/pkg/flags/names" + "github.com/spf13/cobra" +) + +func (cmd *ClusterCmd) registerFlags(c *cobra.Command) { + cmd.registerIdentityFlags(c) + cmd.registerBehaviorFlags(c) + cmd.registerHelmFlags(c) + cmd.registerClusterFlags(c) +} + +func (cmd *ClusterCmd) registerIdentityFlags(c *cobra.Command) { + cliflags.Add( + c, + cliflags.String( + &cmd.Namespace, + names.Namespace, + "loft", + "The namespace to generate the service account in. The namespace will be created if it does not exist", + ), + cliflags.String( + &cmd.ServiceAccount, + names.ServiceAccount, + "loft-admin", + "The service account name to create", + ), + cliflags.String( + &cmd.DisplayName, + names.DisplayName, + "", + "The display name to show in the UI for this cluster", + ), + ) +} + +func (cmd *ClusterCmd) registerBehaviorFlags(c *cobra.Command) { + cliflags.Add( + c, + cliflags.Bool( + &cmd.Wait, + names.Wait, + false, + "If true, will wait until the cluster is initialized", + ), + cliflags.Bool( + &cmd.Insecure, + names.Insecure, + false, + "If true, deploys the agent in insecure mode", + ), + ) +} + +func (cmd *ClusterCmd) registerHelmFlags(c *cobra.Command) { + cliflags.Add( + c, + cliflags.String( + &cmd.HelmChartVersion, + names.HelmChartVersion, + "", + "The agent chart version to deploy", + ), + cliflags.String(&cmd.HelmChartPath, names.HelmChartPath, "", "The agent chart to deploy"), + cliflags.StringArray( + &cmd.HelmSet, + names.HelmSet, + []string{}, + "Extra helm values for the agent chart", + ), + cliflags.StringArray( + &cmd.HelmValues, + names.HelmValues, + []string{}, + "Extra helm values for the agent chart", + ), + ) +} + +func (cmd *ClusterCmd) registerClusterFlags(c *cobra.Command) { + cliflags.Add( + c, + cliflags.String( + &cmd.KubeContext, + names.KubeContext, + "", + "The kube context to use for installation", + ), + cliflags.String(&cmd.Host, names.Host, "", "The pro instance to use"), + ) + proflags.BindEnv(c.Flags(), names.Host) +} diff --git a/cmd/pro/start.go b/cmd/pro/start.go index f307cd0cd..e11cfeb76 100644 --- a/cmd/pro/start.go +++ b/cmd/pro/start.go @@ -24,8 +24,6 @@ import ( loftclientset "github.com/devsy-org/api/pkg/clientset/versioned" proflags "github.com/devsy-org/devsy/cmd/pro/flags" "github.com/devsy-org/devsy/pkg/config" - cliflags "github.com/devsy-org/devsy/pkg/flags" - "github.com/devsy-org/devsy/pkg/flags/names" "github.com/devsy-org/devsy/pkg/hash" "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/machineid" @@ -108,99 +106,7 @@ func NewStartCmd(flags *proflags.GlobalFlags) *cobra.Command { }, } - cliflags.Add( - startCmd, - cliflags.Bool( - &cmd.Docker, - names.Docker, - false, - "If enabled will try to deploy Devsy Pro to the local docker installation.", - ), - cliflags.String(&cmd.DockerImage, names.DockerImage, "", "The docker image to install."), - cliflags.StringArray(&cmd.DockerArgs, names.DockerArg, []string{}, "Extra docker args"), - cliflags.String( - &cmd.Context, - names.Context, - "", - "The kube context to use for installation", - ), - cliflags.String( - &cmd.Namespace, - names.Namespace, - config.ProReleaseName, - "The namespace to install into", - ), - cliflags.String( - &cmd.Host, - names.Host, - "", - "Provide a hostname to enable ingress and configure its hostname", - ), - cliflags.String( - &cmd.Password, - names.Password, - "", - "The password to use for the admin account. (If empty this will be the namespace UID)", - ), - cliflags.String(&cmd.Version, names.Version, "", "The version to install"), - cliflags.String( - &cmd.Values, - names.Values, - "", - "Path to a file for extra helm chart values", - ), - cliflags.Bool( - &cmd.ReuseValues, - names.ReuseValues, - true, - "Reuse previous helm values on upgrade", - ), - cliflags.Bool( - &cmd.Upgrade, - names.Upgrade, - false, - "If true, will try to upgrade the release", - ), - cliflags.String(&cmd.Email, names.Email, "", "The email to use for the installation"), - cliflags.Bool( - &cmd.Reset, - names.Reset, - false, - "If true, an existing instance will be deleted before installing Devsy Pro", - ), - cliflags.Bool( - &cmd.NoWait, - names.NoWait, - false, - "If true, will not wait after installing it", - ), - cliflags.Bool( - &cmd.NoTunnel, - names.NoTunnel, - false, - "If true, will not create a loft.host tunnel for this installation", - ), - cliflags.Bool( - &cmd.NoLogin, - names.NoLogin, - false, - "If true, will not login to a Devsy Pro instance on start", - ), - cliflags.String( - &cmd.ChartPath, - names.ChartPath, - "", - "The local chart path to deploy Devsy Pro", - ), - cliflags.String( - &cmd.ChartRepo, - names.ChartRepo, - "https://charts.devsy.sh/", - "The chart repo to deploy Devsy Pro", - ), - ) - - proflags.BindEnv(startCmd.Flags(), names.Host) + cmd.registerFlags(startCmd) return startCmd } @@ -538,15 +444,35 @@ func (cmd *StartCmd) confirmPortForwardIfUnreachable( } func (cmd *StartCmd) successRemote(ctx context.Context, host string) error { - printSuccess := func() { - url := "https://" + host + ready, err := isHostReachable(ctx, host) + if err != nil { + return err + } else if ready { + cmd.printRemoteSuccessMessage(host) + return nil + } - password := cmd.Password - if password == "" { - password = passwordChangedHint - } + printDNSConfigurationRequired(host) + + if err := waitForHostReachable(ctx, host); err != nil { + return err + } + + log.Info("Devsy Pro is reachable at https://" + host) - fmt.Fprintf(os.Stderr, ` + cmd.printRemoteSuccessMessage(host) + return nil +} + +func (cmd *StartCmd) printRemoteSuccessMessage(host string) { + url := "https://" + host + + password := cmd.Password + if password == "" { + password = passwordChangedHint + } + + fmt.Fprintf(os.Stderr, ` ########################## LOGIN ############################ @@ -566,20 +492,13 @@ Devsy Pro was successfully installed and can now be reached at: %s Thanks for using Devsy Pro! `, - greenBold(url), - greenBold("devsy pro login "+url), - "https://devsy.sh/docs/administration/ssl", - url) - } - ready, err := isHostReachable(ctx, host) - if err != nil { - return err - } else if ready { - printSuccess() - return nil - } + greenBold(url), + greenBold("devsy pro login "+url), + "https://devsy.sh/docs/administration/ssl", + url) +} - // Print DNS Configuration +func printDNSConfigurationRequired(host string) { fmt.Fprint(os.Stderr, ` ################################### DNS CONFIGURATION REQUIRED ################################## @@ -604,7 +523,10 @@ The command will wait until Devsy Pro is reachable under the host. log.Info( "Waiting for you to configure DNS, so Devsy Pro can be reached on https://" + host, ) - err = wait.PollUntilContextTimeout( +} + +func waitForHostReachable(ctx context.Context, host string) error { + return wait.PollUntilContextTimeout( ctx, 5*time.Second, platform.Timeout(), @@ -613,14 +535,6 @@ The command will wait until Devsy Pro is reachable under the host. return isHostReachable(ctx, host) }, ) - if err != nil { - return err - } - - log.Info("Devsy Pro is reachable at https://" + host) - - printSuccess() - return nil } func (cmd *StartCmd) successLocal() error { @@ -1538,6 +1452,26 @@ func uninstall( kubeClient kubernetes.Interface, restConfig *rest.Config, kubeContext, namespace string, +) error { + if err := runHelmUninstall(ctx, kubeClient, kubeContext, namespace); err != nil { + return err + } + + if err := cleanupProResources(ctx, kubeClient, restConfig, namespace); err != nil { + return err + } + + fmt.Fprint(os.Stderr, "\n") + log.Info("uninstalled Devsy Pro") + fmt.Fprint(os.Stderr, "\n") + + return nil +} + +func runHelmUninstall( + ctx context.Context, + kubeClient kubernetes.Interface, + kubeContext, namespace string, ) error { releaseName, err := resolveReleaseName(ctx, kubeClient, namespace) if err != nil { @@ -1559,7 +1493,15 @@ func uninstall( log.Errorf("error during helm command: %s (%v)", string(output), err) } - // we also cleanup the validating webhook configuration and apiservice + return nil +} + +func cleanupProResources( + ctx context.Context, + kubeClient kubernetes.Interface, + restConfig *rest.Config, + namespace string, +) error { apiRegistrationClient, err := clientset.NewForConfig(restConfig) if err != nil { return err @@ -1574,12 +1516,20 @@ func uninstall( return err } - err = deleteUser(ctx, restConfig, "admin") - if err != nil { + if err := deleteUser(ctx, restConfig, "admin"); err != nil { return err } - err = deleteIgnoreNotFound( + return deleteRemainingAgentResources(ctx, kubeClient, apiRegistrationClient, namespace) +} + +func deleteRemainingAgentResources( + ctx context.Context, + kubeClient kubernetes.Interface, + apiRegistrationClient *clientset.Clientset, + namespace string, +) error { + return deleteIgnoreNotFound( func() error { return kubeClient.CoreV1(). Secrets(namespace). @@ -1616,15 +1566,6 @@ func uninstall( Delete(ctx, "loft-applied-defaults", metav1.DeleteOptions{}) }, ) - if err != nil { - return err - } - - fmt.Fprint(os.Stderr, "\n") - log.Info("uninstalled Devsy Pro") - fmt.Fprint(os.Stderr, "\n") - - return nil } func resolveReleaseName( diff --git a/cmd/pro/start_flags.go b/cmd/pro/start_flags.go new file mode 100644 index 000000000..6c0c2b175 --- /dev/null +++ b/cmd/pro/start_flags.go @@ -0,0 +1,137 @@ +package pro + +import ( + proflags "github.com/devsy-org/devsy/cmd/pro/flags" + "github.com/devsy-org/devsy/pkg/config" + cliflags "github.com/devsy-org/devsy/pkg/flags" + "github.com/devsy-org/devsy/pkg/flags/names" + "github.com/spf13/cobra" +) + +func (cmd *StartCmd) registerFlags(startCmd *cobra.Command) { + cmd.registerDockerFlags(startCmd) + cmd.registerClusterFlags(startCmd) + cmd.registerChartFlags(startCmd) + cmd.registerAuthFlags(startCmd) + cmd.registerLifecycleFlags(startCmd) +} + +func (cmd *StartCmd) registerDockerFlags(startCmd *cobra.Command) { + cliflags.Add( + startCmd, + cliflags.Bool( + &cmd.Docker, + names.Docker, + false, + "If enabled will try to deploy Devsy Pro to the local docker installation.", + ), + cliflags.String(&cmd.DockerImage, names.DockerImage, "", "The docker image to install."), + cliflags.StringArray(&cmd.DockerArgs, names.DockerArg, []string{}, "Extra docker args"), + ) +} + +func (cmd *StartCmd) registerClusterFlags(startCmd *cobra.Command) { + cliflags.Add( + startCmd, + cliflags.String( + &cmd.Context, + names.Context, + "", + "The kube context to use for installation", + ), + cliflags.String( + &cmd.Namespace, + names.Namespace, + config.ProReleaseName, + "The namespace to install into", + ), + cliflags.String( + &cmd.Host, + names.Host, + "", + "Provide a hostname to enable ingress and configure its hostname", + ), + ) + + proflags.BindEnv(startCmd.Flags(), names.Host) +} + +func (cmd *StartCmd) registerChartFlags(startCmd *cobra.Command) { + cliflags.Add( + startCmd, + cliflags.String(&cmd.Version, names.Version, "", "The version to install"), + cliflags.String( + &cmd.Values, + names.Values, + "", + "Path to a file for extra helm chart values", + ), + cliflags.Bool( + &cmd.ReuseValues, + names.ReuseValues, + true, + "Reuse previous helm values on upgrade", + ), + cliflags.String( + &cmd.ChartPath, + names.ChartPath, + "", + "The local chart path to deploy Devsy Pro", + ), + cliflags.String( + &cmd.ChartRepo, + names.ChartRepo, + "https://charts.devsy.sh/", + "The chart repo to deploy Devsy Pro", + ), + ) +} + +func (cmd *StartCmd) registerAuthFlags(startCmd *cobra.Command) { + cliflags.Add( + startCmd, + cliflags.String( + &cmd.Password, + names.Password, + "", + "The password to use for the admin account. (If empty this will be the namespace UID)", + ), + cliflags.String(&cmd.Email, names.Email, "", "The email to use for the installation"), + ) +} + +func (cmd *StartCmd) registerLifecycleFlags(startCmd *cobra.Command) { + cliflags.Add( + startCmd, + cliflags.Bool( + &cmd.Upgrade, + names.Upgrade, + false, + "If true, will try to upgrade the release", + ), + cliflags.Bool( + &cmd.Reset, + names.Reset, + false, + "If true, an existing instance will be deleted before installing Devsy Pro", + ), + cliflags.Bool( + &cmd.NoWait, + names.NoWait, + false, + "If true, will not wait after installing it", + ), + cliflags.Bool( + &cmd.NoTunnel, + names.NoTunnel, + false, + "If true, will not create a loft.host tunnel for this installation", + ), + cliflags.Bool( + &cmd.NoLogin, + names.NoLogin, + false, + "If true, will not login to a Devsy Pro instance on start", + ), + ) +} diff --git a/cmd/provider/init.go b/cmd/provider/init.go index 0e75d281f..4e2697124 100644 --- a/cmd/provider/init.go +++ b/cmd/provider/init.go @@ -6,8 +6,6 @@ import ( "github.com/devsy-org/devsy/cmd/completion" "github.com/devsy-org/devsy/cmd/flags" "github.com/devsy-org/devsy/pkg/config" - cliflags "github.com/devsy-org/devsy/pkg/flags" - "github.com/devsy-org/devsy/pkg/flags/names" "github.com/devsy-org/devsy/pkg/status" "github.com/devsy-org/devsy/pkg/workspace" "github.com/spf13/cobra" @@ -75,15 +73,6 @@ func NewInitCmd(f *flags.GlobalFlags) *cobra.Command { ) }, } - cliflags.Add(initCmd, - cliflags.Bool(&cmd.Reset, names.Reset, false, - "Discard previously stored option answers and re-prompt from scratch"), - cliflags.Bool(&cmd.SingleMachine, names.SingleMachine, false, - "Use a single machine for all workspaces"), - cliflags.StringArray(&cmd.Options, names.Option, nil, - "Provider option in the form KEY=VALUE").Shorthand("o"), - cliflags.Bool(&cmd.SkipInit, names.SkipInit, false, - "Skip provider init (testing only)").Hidden(), - ) + cmd.registerFlags(initCmd) return initCmd } diff --git a/cmd/provider/init_flags.go b/cmd/provider/init_flags.go new file mode 100644 index 000000000..a2be5cfcc --- /dev/null +++ b/cmd/provider/init_flags.go @@ -0,0 +1,30 @@ +package provider + +import ( + cliflags "github.com/devsy-org/devsy/pkg/flags" + "github.com/devsy-org/devsy/pkg/flags/names" + "github.com/spf13/cobra" +) + +func (cmd *InitCmd) registerFlags(initCmd *cobra.Command) { + cmd.registerOptionFlags(initCmd) + cmd.registerTestingFlags(initCmd) +} + +func (cmd *InitCmd) registerOptionFlags(initCmd *cobra.Command) { + cliflags.Add(initCmd, + cliflags.Bool(&cmd.Reset, names.Reset, false, + "Discard previously stored option answers and re-prompt from scratch"), + cliflags.Bool(&cmd.SingleMachine, names.SingleMachine, false, + "Use a single machine for all workspaces"), + cliflags.StringArray(&cmd.Options, names.Option, nil, + "Provider option in the form KEY=VALUE").Shorthand("o"), + ) +} + +func (cmd *InitCmd) registerTestingFlags(initCmd *cobra.Command) { + cliflags.Add(initCmd, + cliflags.Bool(&cmd.SkipInit, names.SkipInit, false, + "Skip provider init (testing only)").Hidden(), + ) +} diff --git a/cmd/workspace/ssh.go b/cmd/workspace/ssh.go index 162374954..745ddc6de 100644 --- a/cmd/workspace/ssh.go +++ b/cmd/workspace/ssh.go @@ -84,6 +84,21 @@ func NewSSHCmd(f *flags.GlobalFlags) *cobra.Command { }, } + cmd.registerFlags(sshCmd) + + return sshCmd +} + +func (cmd *SSHCmd) registerFlags(sshCmd *cobra.Command) { + cmd.registerPortForwardingFlags(sshCmd) + cmd.registerEnvFlags(sshCmd) + cmd.registerSessionFlags(sshCmd) + cmd.registerAgentForwardingFlags(sshCmd) + cmd.registerServiceFlags(sshCmd) + cmd.registerTerminalFlags(sshCmd) +} + +func (cmd *SSHCmd) registerPortForwardingFlags(sshCmd *cobra.Command) { cliflags.Add( sshCmd, cliflags.StringArray( @@ -102,16 +117,28 @@ func NewSSHCmd(f *flags.GlobalFlags) *cobra.Command { "host are to be reverse forwarded to the given host and port, or Unix socket, on the remote side.", ). Shorthand("R"), - cliflags.StringArray(&cmd.SendEnvVars, names.SendEnv, nil, - "Specifies which local env variables shall be sent to the container."), - cliflags.StringArray(&cmd.SetEnvVars, names.SetEnv, nil, - "Specifies env variables to be set in the container."), cliflags.String( &cmd.ForwardPortsTimeout, names.ForwardPortsTimeout, "", "Specifies the timeout after which the command should terminate when the ports are unused.", ), + ) +} + +func (cmd *SSHCmd) registerEnvFlags(sshCmd *cobra.Command) { + cliflags.Add( + sshCmd, + cliflags.StringArray(&cmd.SendEnvVars, names.SendEnv, nil, + "Specifies which local env variables shall be sent to the container."), + cliflags.StringArray(&cmd.SetEnvVars, names.SetEnv, nil, + "Specifies env variables to be set in the container."), + ) +} + +func (cmd *SSHCmd) registerSessionFlags(sshCmd *cobra.Command) { + cliflags.Add( + sshCmd, cliflags.String( &cmd.Command, names.Command, @@ -120,6 +147,18 @@ func NewSSHCmd(f *flags.GlobalFlags) *cobra.Command { ), cliflags.String(&cmd.User, names.User, "", "The user of the workspace to use"), cliflags.String(&cmd.WorkDir, names.Workdir, "", "The working directory in the container"), + cliflags.Bool( + &cmd.Stdio, + names.Stdio, + false, + "If true will tunnel connection through stdout and stdin", + ), + ) +} + +func (cmd *SSHCmd) registerAgentForwardingFlags(sshCmd *cobra.Command) { + cliflags.Add( + sshCmd, cliflags.Bool(&cmd.AgentForwarding, names.AgentForwarding, true, "If true forward the local ssh keys to the remote machine"), cliflags.String(&cmd.ReuseSSHAuthSock, names.ReuseSSHAuthSock, "", @@ -128,25 +167,29 @@ func NewSSHCmd(f *flags.GlobalFlags) *cobra.Command { Hidden(), cliflags.Bool(&cmd.GPGAgentForwarding, names.SSHGPGForwarding, false, "Forward the local gpg-agent to the remote machine"), - cliflags.Bool( - &cmd.Stdio, - names.Stdio, - false, - "If true will tunnel connection through stdout and stdin", - ), + cliflags.String(&cmd.GitSSHSigningKey, names.GitSSHSigningKey, "", + "The SSH signing key to use for git commit signing inside the workspace"), + ) +} + +func (cmd *SSHCmd) registerServiceFlags(sshCmd *cobra.Command) { + cliflags.Add( + sshCmd, cliflags.Bool(&cmd.StartServices, names.StartServices, true, "If false will not start any port-forwarding or git / docker credentials helper"), cliflags.Duration(&cmd.SSHKeepAliveInterval, names.SSHKeepAliveInterval, 55*time.Second, "How often should keepalive request be made (55s)"), - cliflags.String(&cmd.GitSSHSigningKey, names.GitSSHSigningKey, "", - "The SSH signing key to use for git commit signing inside the workspace"), + ) +} + +func (cmd *SSHCmd) registerTerminalFlags(sshCmd *cobra.Command) { + cliflags.Add( + sshCmd, cliflags.String(&cmd.TermMode, names.TermMode, machine.TermModeAuto, "PTY TERM selection mode: auto, strict, fallback"), cliflags.Bool(&cmd.InstallTerminfo, names.InstallTerminfo, false, "Install local TERM terminfo on remote before PTY"), ) - - return sshCmd } // Run runs the command logic. @@ -388,17 +431,9 @@ func (cmd *SSHCmd) startTunnel( } cmd.startTunnelServices(ctx, devsyConfig, containerClient, workspaceClient) - // buildSSHServerCommand runs `devsy internal ssh-server`, which always - // logs structured JSON on stderr; PipeJSONStream re-emits each line at - // its original level instead of double-wrapping it as another log entry. - writer, writerDone := log.PipeJSONStream() - defer func() { - _ = writer.Close() - <-writerDone - }() - gpgTunnel := newGPGTunnel(cmd, devsyConfig) - defer runGPGTunnelInBackground(ctx, gpgTunnel, containerClient)() + writer, cleanup := cmd.setupTunnelWriter(ctx, devsyConfig, containerClient) + defer cleanup() workdir := resolveWorkdir(cmd.WorkDir, workspaceClient) @@ -422,6 +457,47 @@ func (cmd *SSHCmd) startTunnel( }) } + return cmd.runInteractiveTunnelSession( + ctx, + devsyConfig, + containerClient, + command, + envVars, + writer, + ) +} + +// setupTunnelWriter wires up the JSON log pipe and GPG agent tunnel shared by +// both tunnel modes. buildSSHServerCommand runs `devsy internal ssh-server`, +// which always logs structured JSON on stderr; PipeJSONStream re-emits each +// line at its original level instead of double-wrapping it as another log +// entry. The returned cleanup func stops the GPG tunnel before closing and +// draining the writer, matching the original defer ordering. +func (cmd *SSHCmd) setupTunnelWriter( + ctx context.Context, + devsyConfig *config.Config, + containerClient *ssh.Client, +) (io.Writer, func()) { + writer, writerDone := log.PipeJSONStream() + + gpgTunnel := newGPGTunnel(cmd, devsyConfig) + stopGPGTunnel := runGPGTunnelInBackground(ctx, gpgTunnel, containerClient) + + return writer, func() { + stopGPGTunnel() + _ = writer.Close() + <-writerDone + } +} + +func (cmd *SSHCmd) runInteractiveTunnelSession( + ctx context.Context, + devsyConfig *config.Config, + containerClient *ssh.Client, + command string, + envVars map[string]string, + writer io.Writer, +) error { return machine.StartSSHSession(ctx, machine.StartSSHSessionOptions{ User: cmd.User, Command: cmd.Command, diff --git a/cmd/workspace/up/up_flags.go b/cmd/workspace/up/up_flags.go index d4e5307ed..20a8675ec 100644 --- a/cmd/workspace/up/up_flags.go +++ b/cmd/workspace/up/up_flags.go @@ -172,6 +172,12 @@ func (cmd *UpCmd) registerPodmanFlags(upCmd *cobra.Command) { } func (cmd *UpCmd) registerWorkspaceFlags(upCmd *cobra.Command) { + cmd.registerWorkspaceIdentityFlags(upCmd) + cmd.registerWorkspaceSecretsFlags(upCmd) + cmd.registerWorkspaceRuntimeFlags(upCmd) +} + +func (cmd *UpCmd) registerWorkspaceIdentityFlags(upCmd *cobra.Command) { flags.Add( upCmd, flags.String(&cmd.ID, names.ID, "", "ID for the workspace"), @@ -206,6 +212,12 @@ func (cmd *UpCmd) registerWorkspaceFlags(upCmd *cobra.Command) { "report it instead"), flags.StringSlice(&cmd.PrebuildRepositories, names.PrebuildRepo, nil, "Docker repository hosting prebuilds for this workspace"), + ) +} + +func (cmd *UpCmd) registerWorkspaceSecretsFlags(upCmd *cobra.Command) { + flags.Add( + upCmd, flags.StringArray(&cmd.WorkspaceEnv, names.WorkspaceEnv, nil, "Env var for the workspace (KEY=VALUE, repeatable)"), flags.StringSlice(&cmd.WorkspaceEnvFile, names.WorkspaceEnvFile, nil, @@ -230,6 +242,12 @@ func (cmd *UpCmd) registerWorkspaceFlags(upCmd *cobra.Command) { "Env var for workspace initialization (KEY=VALUE, repeatable)"), flags.Bool(&cmd.DisableDaemon, names.DisableDaemon, false, "Do not install the activity-tracking daemon on the target machine"), + ) +} + +func (cmd *UpCmd) registerWorkspaceRuntimeFlags(upCmd *cobra.Command) { + flags.Add( + upCmd, flags.StringArray(&cmd.CacheFrom, names.CacheFrom, nil, "Build cache source (e.g. myregistry.io/cache:latest or type=registry,ref=...); "+ "takes priority over devcontainer.json build.cacheFrom"), From dec81d3126836654473feaca30cb87b4f7f22935 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 6 Aug 2026 15:13:14 +0000 Subject: [PATCH 2/5] fix(lint): resolve funlen findings in business logic Group B of the funlen backlog: real business-logic functions, extracted into sensibly-named helpers per logical phase (same approach as the ResolvePortAttribute cyclop fix). Behavior preserved exactly; callers of every touched exported function were checked via grep before any signature change (none required signature changes). - cmd/internal/agentcontainer/setup.go: streamMount -> streamMountFromPlatform/ buildPlatformDownloadRequest/streamMountFromTunnel - cmd/pro/start.go: successRemote -> printRemoteSuccessMessage/ printDNSConfigurationRequired/waitForHostReachable; uninstall -> runHelmUninstall/cleanupProResources/deleteRemainingAgentResources - cmd/workspace/logs.go: Run -> getWorkspaceClient/injectLogsAgent - cmd/workspace/ssh.go: startTunnel -> setupTunnelWriter/ runInteractiveTunnelSession - pkg/client/clientimplementation/daemonclient/form.go: createInstanceInteractive -> selectProjectClusterTemplate/ resolveNewInstanceParameters/buildNewInstance - pkg/client/clientimplementation/daemonclient/up.go: printLogs -> openTaskLogsStream/newLogScanner/logOutputStreams/streamLogMessages - pkg/devcontainer/compose_build.go: buildAndExtendDockerCompose -> resolveComposeBuildTarget/runComposeExtendedBuild - pkg/devcontainer/config/merge.go: MergeConfiguration (exported, all 6 call sites verified unchanged) -> ensureImageMetadataEntries/ newMergedDevContainerConfig/mergeRuntimeFields/mergeLifecycleHookFields/ mergeUserAndEnvFields/mergePortsAndShutdownFields - pkg/devcontainer/config/userenvprobe.go: ProbeUserEnv (exported, sole caller verified unchanged) -> resolveUserEnvProbe/probeUserEnvWithFallback - pkg/inject/inject.go: Inject suppressed with //nolint:funlen -- this is the same legacy shell injection path already frozen and suppressed for staticcheck (SA1019) in pkg/agent/inject.go; a functional decomposition of dead-end code awaiting AgentDelivery migration isn't worth the risk. - pkg/options/resolve.go: ResolveOptions -> applyResolvedProviderOptions - pkg/platform/form/form.go: CreateInstance and UpdateInstance (exported, all 5 call sites verified unchanged) -> shared resolveTemplateParameters/ runParameterForm helpers plus per-function runCreateSelectionForm/ renderedParametersForCreate/buildCreatedInstance and selectUpdateTemplate; also removes pre-existing duplication between the two functions - pkg/platform/kubeconfig.go: kubeConfigForSpaceInstance and kubeConfigForVirtualClusterInstance share a new kubeConfigViaAccessKey helper for their near-identical access-key path (previously duplicated), plus per-function directClusterEndpointKubeConfigForSpace/ directVirtualClusterKubeConfig/newVClusterKubeConfigRequest - pkg/ssh/server/ssh.go: NewServer -> buildSSHServer --- cmd/workspace/logs.go | 92 ++++--- .../clientimplementation/daemonclient/form.go | 94 +++++-- .../clientimplementation/daemonclient/up.go | 114 ++++++-- pkg/devcontainer/compose_build.go | 80 ++++-- pkg/devcontainer/config/merge.go | 77 ++++-- pkg/devcontainer/config/userenvprobe.go | 80 +++--- pkg/inject/inject.go | 2 + pkg/options/resolve.go | 56 ++-- pkg/platform/form/form.go | 190 ++++++++----- pkg/platform/kubeconfig.go | 252 +++++++++++------- pkg/ssh/server/ssh.go | 73 ++--- 11 files changed, 746 insertions(+), 364 deletions(-) diff --git a/cmd/workspace/logs.go b/cmd/workspace/logs.go index 4b8dac74c..bdb9c3286 100644 --- a/cmd/workspace/logs.go +++ b/cmd/workspace/logs.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "os" + "time" "github.com/devsy-org/devsy/cmd/completion" "github.com/devsy-org/devsy/cmd/flags" @@ -58,18 +59,9 @@ func (cmd *LogsCmd) Run(ctx context.Context, args []string) error { return err } - baseClient, err := workspace.Get(ctx, workspace.GetOptions{ - DevsyConfig: devsyConfig, - Args: args, - Owner: cmd.Owner, - }) + client, err := cmd.getWorkspaceClient(ctx, devsyConfig, args) if err != nil { - return fmt.Errorf("get workspace for logs: %w", err) - } - - client, ok := baseClient.(clientpkg.WorkspaceClient) - if !ok { - return fmt.Errorf("this command is not supported for proxy providers") + return err } sshServerCmd := fmt.Sprintf("'%s' internal ssh-server --stdio", client.AgentPath()) @@ -87,28 +79,7 @@ func (cmd *LogsCmd) Run(ctx context.Context, args []string) error { return pb.RunPair(ctx, func(ctx context.Context, stdin, stdout *os.File) error { - stderr := log.Writer(log.LevelDebug) - defer func() { _ = stderr.Close() }() - - return agent.InjectAgent(&agent.InjectOptions{ - Ctx: ctx, - Exec: func(ctx context.Context, command string, stdinR io.Reader, stdoutW io.Writer, stderrW io.Writer) error { - return client.Command(ctx, clientpkg.CommandOptions{ - Command: command, - Stdin: stdinR, - Stdout: stdoutW, - Stderr: stderrW, - }) - }, - IsLocal: client.AgentLocal(), - RemoteAgentPath: client.AgentPath(), - DownloadURL: client.AgentURL(), - Command: sshServerCmd, - Stdin: stdin, - Stdout: stdout, - Stderr: stderr, - Timeout: timeout, - }) + return injectLogsAgent(ctx, client, sshServerCmd, timeout, stdin, stdout) }, func(ctx context.Context, stdout, stdin *os.File) error { return runLogsSession(stdout, stdin, client) @@ -116,6 +87,61 @@ func (cmd *LogsCmd) Run(ctx context.Context, args []string) error { ) } +// getWorkspaceClient resolves args to a WorkspaceClient, rejecting proxy providers +// which don't support log streaming. +func (cmd *LogsCmd) getWorkspaceClient( + ctx context.Context, devsyConfig *config.Config, args []string, +) (clientpkg.WorkspaceClient, error) { + baseClient, err := workspace.Get(ctx, workspace.GetOptions{ + DevsyConfig: devsyConfig, + Args: args, + Owner: cmd.Owner, + }) + if err != nil { + return nil, fmt.Errorf("get workspace for logs: %w", err) + } + + client, ok := baseClient.(clientpkg.WorkspaceClient) + if !ok { + return nil, fmt.Errorf("this command is not supported for proxy providers") + } + + return client, nil +} + +// injectLogsAgent injects the devsy agent binary over stdin/stdout and runs the +// remote ssh-server that runLogsSession then connects to. +func injectLogsAgent( + ctx context.Context, + client clientpkg.WorkspaceClient, + sshServerCmd string, + timeout time.Duration, + stdin, stdout *os.File, +) error { + stderr := log.Writer(log.LevelDebug) + defer func() { _ = stderr.Close() }() + + return agent.InjectAgent(&agent.InjectOptions{ + Ctx: ctx, + Exec: func(ctx context.Context, command string, stdinR io.Reader, stdoutW io.Writer, stderrW io.Writer) error { + return client.Command(ctx, clientpkg.CommandOptions{ + Command: command, + Stdin: stdinR, + Stdout: stdoutW, + Stderr: stderrW, + }) + }, + IsLocal: client.AgentLocal(), + RemoteAgentPath: client.AgentPath(), + DownloadURL: client.AgentURL(), + Command: sshServerCmd, + Stdin: stdin, + Stdout: stdout, + Stderr: stderr, + Timeout: timeout, + }) +} + func runLogsSession(stdout, stdin *os.File, client clientpkg.WorkspaceClient) error { sshClient, err := ssh.StdioClientWithUser(stdout, stdin, "", false) if err != nil { diff --git a/pkg/client/clientimplementation/daemonclient/form.go b/pkg/client/clientimplementation/daemonclient/form.go index 57eb40cdc..a0e5a7aed 100644 --- a/pkg/client/clientimplementation/daemonclient/form.go +++ b/pkg/client/clientimplementation/daemonclient/form.go @@ -29,19 +29,58 @@ func createInstanceInteractive( formCtx, cancelForm := context.WithCancel(ctx) defer cancelForm() + selectedProject, selectedCluster, selectedTemplate, selectedTemplateVersion, err := selectProjectClusterTemplate( + ctx, + formCtx, + baseClient, + cancelForm, + ) + if err != nil { + return nil, err + } + + renderedParameters, err := resolveNewInstanceParameters( + formCtx, + selectedTemplate, + selectedTemplateVersion, + ) + if err != nil { + return nil, err + } + + return buildNewInstance( + id, + uid, + source, + picture, + selectedProject, + selectedCluster, + selectedTemplate, + selectedTemplateVersion, + renderedParameters, + ), nil +} + +func selectProjectClusterTemplate( + ctx, formCtx context.Context, + baseClient platformclient.Client, + cancelForm CancelFunc, +) (*managementv1.Project, *managementv1.Cluster, *managementv1.DevsyWorkspaceTemplate, string, error) { var selectedCluster *managementv1.Cluster var selectedProject *managementv1.Project var selectedTemplate *managementv1.DevsyWorkspaceTemplate selectedTemplateVersion := "" - projectOptions, err := projectOptions(ctx, baseClient) + + options, err := projectOptions(ctx, baseClient) if err != nil { - return nil, err + return nil, nil, nil, "", err } + err = huh.NewForm( huh.NewGroup( huh.NewSelect[*managementv1.Project](). Title("Project"). - Options(projectOptions...). + Options(options...). Value(&selectedProject), huh.NewSelect[*managementv1.Cluster](). Title("Cluster"). @@ -66,34 +105,47 @@ func createInstanceInteractive( ), ).RunWithContext(formCtx) if err != nil { - return nil, err + return nil, nil, nil, "", err } + return selectedProject, selectedCluster, selectedTemplate, selectedTemplateVersion, nil +} + +func resolveNewInstanceParameters( + formCtx context.Context, + selectedTemplate *managementv1.DevsyWorkspaceTemplate, + selectedTemplateVersion string, +) (string, error) { parameters := selectedTemplate.Spec.Parameters if len(selectedTemplate.GetVersions()) > 0 { + var err error parameters, err = list.GetTemplateParameters(selectedTemplate, selectedTemplateVersion) if err != nil { - return nil, err + return "", err } } + if len(parameters) == 0 { + return "", nil + } - renderedParameters := "" - if len(parameters) > 0 { - fieldParameters := prepareParameters(parameters) - err = huh.NewForm( - huh.NewGroup(parameterFields(fieldParameters)...), - ).RunWithContext(formCtx) - if err != nil { - return nil, err - } - - renderedParameters, err = renderParameters(fieldParameters) - if err != nil { - return nil, err - } + fieldParameters := prepareParameters(parameters) + if err := huh.NewForm( + huh.NewGroup(parameterFields(fieldParameters)...), + ).RunWithContext(formCtx); err != nil { + return "", err } - instance := &managementv1.DevsyWorkspaceInstance{ + return renderParameters(fieldParameters) +} + +func buildNewInstance( + id, uid, source, picture string, + selectedProject *managementv1.Project, + selectedCluster *managementv1.Cluster, + selectedTemplate *managementv1.DevsyWorkspaceTemplate, + selectedTemplateVersion, renderedParameters string, +) *managementv1.DevsyWorkspaceInstance { + return &managementv1.DevsyWorkspaceInstance{ ObjectMeta: metav1.ObjectMeta{ GenerateName: encoding.SafeConcatNameMax([]string{id}, 53) + "-", Namespace: project.ProjectNamespace(selectedProject.GetName()), @@ -123,8 +175,6 @@ func createInstanceInteractive( }, }, } - - return instance, nil } func updateInstanceInteractive( diff --git a/pkg/client/clientimplementation/daemonclient/up.go b/pkg/client/clientimplementation/daemonclient/up.go index 14bd8572a..fdbfdac01 100644 --- a/pkg/client/clientimplementation/daemonclient/up.go +++ b/pkg/client/clientimplementation/daemonclient/up.go @@ -357,7 +357,37 @@ func printLogs( taskID string, reporter status.Reporter, ) (int, error) { - // get logs reader + logsReader, err := openTaskLogsStream(ctx, managementClient, workspace, taskID) + if err != nil { + return -1, err + } + defer func() { _ = logsReader.Close() }() + + scanner := newLogScanner(logsReader) + + streams := newLogOutputStreams(reporter) + defer streams.Close() + + exitCode, done, err := streamLogMessages(scanner, streams.stdout(), streams.stderrStreamer) + if done { + return exitCode, err + } + if err := scanner.Err(); err != nil { + if errors.Is(err, context.Canceled) { + return 0, nil + } + return -1, fmt.Errorf("logs reader error: %w", err) + } + + return 0, nil +} + +func openTaskLogsStream( + ctx context.Context, + managementClient kube.Interface, + workspace *managementv1.DevsyWorkspaceInstance, + taskID string, +) (io.ReadCloser, error) { log.Debugf("printing logs of task: %s", taskID) logsReader, err := managementClient.Loft().ManagementV1().RESTClient().Get(). Namespace(workspace.Namespace). @@ -370,12 +400,13 @@ func printLogs( }, builders.ParameterCodec). Stream(ctx) if err != nil { - return -1, fmt.Errorf("error getting task logs: %w", err) + return nil, fmt.Errorf("error getting task logs: %w", err) } - defer func() { _ = logsReader.Close() }() + return logsReader, nil +} - // create scanner from logs reader - scanner := bufio.NewScanner(logsReader) +func newLogScanner(r io.Reader) *bufio.Scanner { + scanner := bufio.NewScanner(r) // Increase the maximum token size to handle very long lines. // Here, we set a maximum capacity of 1MB. @@ -383,48 +414,73 @@ func printLogs( buf := make([]byte, 1024) // starting buffer size of 1KB scanner.Buffer(buf, maxCapacity) - // create json streamer + return scanner +} + +// logOutputStreams bundles the stdout/stderr JSON streamers used to print +// task logs, plus the status-sniffing writer wrapped around stdout. +type logOutputStreams struct { + stdoutStreamer io.WriteCloser + stdoutDone chan struct{} + stderrStreamer io.WriteCloser + stderrDone chan struct{} + statusWriter *statusSniffingWriter +} + +func newLogOutputStreams(reporter status.Reporter) *logOutputStreams { stdoutStreamer, stdoutDone := log.PipeJSONStream() stderrStreamer, stderrDone := log.PipeJSONStream() - defer func() { - // close the streams - _ = stdoutStreamer.Close() - _ = stderrStreamer.Close() - - // wait for the streams to be closed - <-stdoutDone - <-stderrDone - }() // The remote task runs the same devsy CLI, so its stdout carries the // same NDJSON status lines a local `up` does; sniff them out here. statusWriter := newStatusSniffingWriter(stdoutStreamer, reporter) - defer func() { _ = statusWriter.Close() }() - stdout := io.Writer(statusWriter) - // loop over all lines + return &logOutputStreams{ + stdoutStreamer: stdoutStreamer, + stdoutDone: stdoutDone, + stderrStreamer: stderrStreamer, + stderrDone: stderrDone, + statusWriter: statusWriter, + } +} + +func (s *logOutputStreams) stdout() io.Writer { + return s.statusWriter +} + +func (s *logOutputStreams) Close() { + _ = s.statusWriter.Close() + _ = s.stdoutStreamer.Close() + _ = s.stderrStreamer.Close() + <-s.stdoutDone + <-s.stderrDone +} + +// streamLogMessages reads NDJSON-encoded Message lines from scanner and +// writes their payloads to stdout/stderr until an ExitCode message, a write +// error, or EOF is reached. done reports whether the caller should return +// immediately with (exitCode, err) rather than falling through to +// scanner.Err(). +func streamLogMessages(scanner *bufio.Scanner, stdout, stderr io.Writer) (int, bool, error) { for scanner.Scan() { line := scanner.Text() - // parse message message := &Message{} if err := json.Unmarshal([]byte(line), message); err != nil { - return -1, fmt.Errorf("error parsing JSON from logs reader: %w, line: %s", err, line) + return -1, true, fmt.Errorf( + "error parsing JSON from logs reader: %w, line: %s", + err, + line, + ) } - exitCode, done, err := writeMessage(stdout, stderrStreamer, message) + exitCode, done, err := writeMessage(stdout, stderr, message) if done { - return exitCode, err - } - } - if err := scanner.Err(); err != nil { - if errors.Is(err, context.Canceled) { - return 0, nil + return exitCode, true, err } - return -1, fmt.Errorf("logs reader error: %w", err) } - return 0, nil + return 0, false, nil } func writeMessage(stdout, stderr io.Writer, message *Message) (int, bool, error) { diff --git a/pkg/devcontainer/compose_build.go b/pkg/devcontainer/compose_build.go index 772eb15e9..ce1ff659f 100644 --- a/pkg/devcontainer/compose_build.go +++ b/pkg/devcontainer/compose_build.go @@ -125,39 +125,17 @@ func (r *runner) buildAndExtendDockerCompose( if err != nil { return composeExtendResult{}, err } - extendImageBuildInfo := prepared.extendImageBuildInfo - buildImageName, err := composeBuildImageName( - params.composeHelper, - params.project.Name, - params.composeService, - hasFeatureBuildInfo(extendImageBuildInfo), - ) - if err != nil { - return composeExtendResult{}, err - } - - dockerComposeFilePath, cleanup, err := r.composeFeatureOverride( + buildImageName, dockerComposeFilePath, cleanup, err := r.resolveComposeBuildTarget( prepared, - params.composeService, - buildImageName, + params, ) defer cleanup() if err != nil { return composeExtendResult{buildImageName: buildImageName}, err } - buildArgs := composeBuildArgs(&composeBuildArgsParams{ - projectName: params.project.Name, - globalArgs: params.globalArgs, - overrideComposeFilePath: dockerComposeFilePath, - pull: params.pull, - noCache: params.noCache, - serviceName: params.composeService.Name, - runServices: params.parsedConfig.Config.RunServices, - }) - - if err := r.runComposeBuild(ctx, params.composeHelper, buildArgs); err != nil { + if err := r.runComposeExtendedBuild(ctx, params, dockerComposeFilePath); err != nil { return composeExtendResult{buildImageName: buildImageName}, err } @@ -165,7 +143,7 @@ func (r *runner) buildAndExtendDockerCompose( params.substitutionContext, prepared.imageBuildInfo.Metadata, params.parsedConfig, - extendImageBuildInfo.Features, + prepared.extendImageBuildInfo.Features, ) if err != nil { return composeExtendResult{buildImageName: buildImageName}, err @@ -175,10 +153,58 @@ func (r *runner) buildAndExtendDockerCompose( buildImageName: buildImageName, composeBuildFilePath: dockerComposeFilePath, imageMetadata: imageMetadata, - metadataLabel: extendImageBuildInfo.MetadataLabel, + metadataLabel: prepared.extendImageBuildInfo.MetadataLabel, }, nil } +// resolveComposeBuildTarget determines the image name to build and, when the +// build has features, writes the feature override compose file to build +// against. The returned cleanup func is always non-nil and safe to defer. +func (r *runner) resolveComposeBuildTarget( + prepared preparedComposeBuild, + params *buildAndExtendParams, +) (buildImageName string, dockerComposeFilePath string, cleanup func(), err error) { + cleanup = func() {} + + buildImageName, err = composeBuildImageName( + params.composeHelper, + params.project.Name, + params.composeService, + hasFeatureBuildInfo(prepared.extendImageBuildInfo), + ) + if err != nil { + return buildImageName, "", cleanup, err + } + + dockerComposeFilePath, cleanup, err = r.composeFeatureOverride( + prepared, + params.composeService, + buildImageName, + ) + return buildImageName, dockerComposeFilePath, cleanup, err +} + +// runComposeExtendedBuild assembles the compose build arguments and runs +// "docker compose ... build" against the (possibly feature-extended) compose +// override file. +func (r *runner) runComposeExtendedBuild( + ctx context.Context, + params *buildAndExtendParams, + dockerComposeFilePath string, +) error { + buildArgs := composeBuildArgs(&composeBuildArgsParams{ + projectName: params.project.Name, + globalArgs: params.globalArgs, + overrideComposeFilePath: dockerComposeFilePath, + pull: params.pull, + noCache: params.noCache, + serviceName: params.composeService.Name, + runServices: params.parsedConfig.Config.RunServices, + }) + + return r.runComposeBuild(ctx, params.composeHelper, buildArgs) +} + // composeFeatureOverride builds the feature override compose file when the build // has features, returning the override path (empty when none) and a cleanup // function that is always safe to defer. diff --git a/pkg/devcontainer/config/merge.go b/pkg/devcontainer/config/merge.go index 0e4721fe7..415e8c514 100644 --- a/pkg/devcontainer/config/merge.go +++ b/pkg/devcontainer/config/merge.go @@ -38,19 +38,7 @@ func MergeConfiguration( config *DevContainerConfig, imageMetadataEntries []*ImageMetadata, ) (*MergedDevContainerConfig, error) { - // When no image metadata entries are provided, synthesize one from the - // supplied config so that lifecycle hooks, customizations, mounts, etc. - // declared directly in the devcontainer.json still propagate through the - // merge. Callers that build a proper metadata chain (single image, compose, - // etc.) call AddConfigToImageMetadata first and pass a non-empty list, in - // which case the user config is already represented there. - if len(imageMetadataEntries) == 0 && config != nil { - userMetadata := &ImageMetadata{} - userMetadata.DevContainerConfigBase = config.DevContainerConfigBase - userMetadata.DevContainerActions = config.DevContainerActions - userMetadata.NonComposeBase = config.NonComposeBase - imageMetadataEntries = []*ImageMetadata{userMetadata} - } + imageMetadataEntries = ensureImageMetadataEntries(config, imageMetadataEntries) customizations := collectCustomizations(imageMetadataEntries) @@ -59,8 +47,44 @@ func MergeConfiguration( // reverse the order reversed := ReverseSlice(imageMetadataEntries) - // merge config - mergedConfig := &MergedDevContainerConfig{ + mergedConfig := newMergedDevContainerConfig(copiedConfig, customizations) + + // adjust config + mergeRuntimeFields(mergedConfig, reversed) + mergedConfig.Mounts = mergeMounts(reversed) + mergeLifecycleHookFields(mergedConfig, reversed) + mergeUserAndEnvFields(mergedConfig, reversed, copiedConfig) + mergePortsAndShutdownFields(mergedConfig, reversed, copiedConfig) + + return mergedConfig, nil +} + +// ensureImageMetadataEntries synthesizes an ImageMetadata entry from the +// supplied config when no image metadata entries are provided, so that +// lifecycle hooks, customizations, mounts, etc. declared directly in the +// devcontainer.json still propagate through the merge. Callers that build a +// proper metadata chain (single image, compose, etc.) call +// AddConfigToImageMetadata first and pass a non-empty list, in which case the +// user config is already represented there. +func ensureImageMetadataEntries( + config *DevContainerConfig, + imageMetadataEntries []*ImageMetadata, +) []*ImageMetadata { + if len(imageMetadataEntries) != 0 || config == nil { + return imageMetadataEntries + } + userMetadata := &ImageMetadata{} + userMetadata.DevContainerConfigBase = config.DevContainerConfigBase + userMetadata.DevContainerActions = config.DevContainerActions + userMetadata.NonComposeBase = config.NonComposeBase + return []*ImageMetadata{userMetadata} +} + +func newMergedDevContainerConfig( + copiedConfig *DevContainerConfig, + customizations map[string][]any, +) *MergedDevContainerConfig { + return &MergedDevContainerConfig{ UpdatedConfigProperties: UpdatedConfigProperties{ Customizations: customizations, }, @@ -75,8 +99,9 @@ func MergeConfiguration( // devcontainer.json file. Origin: copiedConfig.Origin, } +} - // adjust config +func mergeRuntimeFields(mergedConfig *MergedDevContainerConfig, reversed []*ImageMetadata) { mergedConfig.Init = some(reversed, func(entry *ImageMetadata) *bool { return entry.Init }) mergedConfig.Privileged = some( reversed, @@ -92,7 +117,9 @@ func MergeConfiguration( reversed, func(entry *ImageMetadata) string { return entry.Entrypoint }, ) - mergedConfig.Mounts = mergeMounts(reversed) +} + +func mergeLifecycleHookFields(mergedConfig *MergedDevContainerConfig, reversed []*ImageMetadata) { mergedConfig.OnCreateCommands = mergeLifestyleHooks( reversed, func(entry *ImageMetadata) types.LifecycleHook { return entry.OnCreateCommand }, @@ -113,6 +140,13 @@ func MergeConfiguration( reversed, func(entry *ImageMetadata) types.LifecycleHook { return entry.PostAttachCommand }, ) +} + +func mergeUserAndEnvFields( + mergedConfig *MergedDevContainerConfig, + reversed []*ImageMetadata, + copiedConfig *DevContainerConfig, +) { mergedConfig.WaitFor = firstString( reversed, func(entry *ImageMetadata) string { return entry.WaitFor }, @@ -139,6 +173,13 @@ func MergeConfiguration( reversed, func(entry *ImageMetadata) map[string]string { return entry.ContainerEnv }, ) +} + +func mergePortsAndShutdownFields( + mergedConfig *MergedDevContainerConfig, + reversed []*ImageMetadata, + copiedConfig *DevContainerConfig, +) { mergedConfig.PortsAttributes = mergeMaps( reversed, func(entry *ImageMetadata) map[string]PortAttribute { return entry.PortsAttributes }, @@ -162,8 +203,6 @@ func MergeConfiguration( if mergedConfig.ShutdownAction == "" { mergedConfig.ShutdownAction = defaultShutdownAction(copiedConfig) } - - return mergedConfig, nil } func collectCustomizations(entries []*ImageMetadata) map[string][]any { diff --git a/pkg/devcontainer/config/userenvprobe.go b/pkg/devcontainer/config/userenvprobe.go index 78736a313..0f61c9b92 100644 --- a/pkg/devcontainer/config/userenvprobe.go +++ b/pkg/devcontainer/config/userenvprobe.go @@ -51,12 +51,7 @@ func ProbeUserEnv( probe string, userName string, ) (map[string]string, error) { - userEnvProbe, err := NewUserEnvProbe(probe) - if err != nil { - log.Warnf("Get user env probe: %v", err) - log.Warnf("Falling back to default user env probe: %s", DefaultUserEnvProbe) - userEnvProbe = DefaultUserEnvProbe - } + userEnvProbe := resolveUserEnvProbe(probe) if userEnvProbe == NoneProbe { return map[string]string{}, nil } @@ -66,6 +61,31 @@ func ProbeUserEnv( return nil, fmt.Errorf("find shell for user %s: %w", userName, err) } + probedEnv := probeUserEnvWithFallback(ctx, userEnvProbe, preferredShell, userName) + if probedEnv == nil { + probedEnv = map[string]string{} + } + + return probedEnv, nil +} + +func resolveUserEnvProbe(probe string) UserEnvProbe { + userEnvProbe, err := NewUserEnvProbe(probe) + if err != nil { + log.Warnf("Get user env probe: %v", err) + log.Warnf("Falling back to default user env probe: %s", DefaultUserEnvProbe) + return DefaultUserEnvProbe + } + + return userEnvProbe +} + +func probeUserEnvWithFallback( + ctx context.Context, + userEnvProbe UserEnvProbe, + preferredShell []string, + userName string, +) map[string]string { log.Debugf( "running user env probe with shell %q, probe %q, user %q and command %q", strings.Join(preferredShell, " "), @@ -82,34 +102,32 @@ func ProbeUserEnv( "cat /proc/self/environ", '\x00', ) - if err != nil { - log.Debugf( - "running user env probe with shell %q, probe %q, user %q and command %q", - strings.Join(preferredShell, " "), - string(userEnvProbe), - userName, - "printenv", - ) - - newProbedEnv, newErr := doProbe( - ctx, - userEnvProbe, - preferredShell, - userName, - "printenv", - '\n', - ) - if newErr != nil { - log.Warnf("failed to probe user environment variables: %v, %v", err, newErr) - } else { - probedEnv = newProbedEnv - } + if err == nil { + return probedEnv } - if probedEnv == nil { - probedEnv = map[string]string{} + + log.Debugf( + "running user env probe with shell %q, probe %q, user %q and command %q", + strings.Join(preferredShell, " "), + string(userEnvProbe), + userName, + "printenv", + ) + + newProbedEnv, newErr := doProbe( + ctx, + userEnvProbe, + preferredShell, + userName, + "printenv", + '\n', + ) + if newErr != nil { + log.Warnf("failed to probe user environment variables: %v, %v", err, newErr) + return probedEnv } - return probedEnv, nil + return newProbedEnv } func parseProbeOutput(out []byte, sep byte) map[string]string { diff --git a/pkg/inject/inject.go b/pkg/inject/inject.go index d332f93f2..5af52c0a0 100644 --- a/pkg/inject/inject.go +++ b/pkg/inject/inject.go @@ -50,6 +50,8 @@ type InjectOptions struct { // Deprecated: Inject is part of the legacy shell injection path. Platform-native AgentDelivery // implementations (LocalDockerDelivery, RemoteDockerDelivery, KubernetesDelivery) are the replacements. +// +//nolint:funlen // legacy shell injection path, frozen pending caller migration to AgentDelivery func Inject(opts InjectOptions) (bool, error) { if err := validateInjectOptions(opts); err != nil { return false, err diff --git a/pkg/options/resolve.go b/pkg/options/resolve.go index aabef7d68..e82dd43a4 100644 --- a/pkg/options/resolve.go +++ b/pkg/options/resolve.go @@ -198,28 +198,48 @@ func ResolveOptions( return nil, err } - // save options in dev config - if devConfig != nil { - devConfig = config.CloneConfig(devConfig) - if devConfig.Current().Providers == nil { - devConfig.Current().Providers = map[string]*config.ProviderConfig{} - } - if devConfig.Current().Providers[providerConfig.Name] == nil { - devConfig.Current().Providers[providerConfig.Name] = &config.ProviderConfig{} - } + if devConfig == nil { + return devConfig, nil + } - providerCfg := devConfig.Current().Providers[providerConfig.Name] - providerCfg.Options = map[string]config.OptionValue{} - maps.Copy(providerCfg.Options, resolvedOptionValues) + return applyResolvedProviderOptions( + devConfig, + providerConfig.Name, + resolvedOptionValues, + dynamicOptionDefinitions, + singleMachine, + ), nil +} - providerCfg.DynamicOptions = config.OptionDefinitions{} - maps.Copy(providerCfg.DynamicOptions, dynamicOptionDefinitions) - if singleMachine != nil { - providerCfg.SingleMachine = *singleMachine - } +// applyResolvedProviderOptions clones devConfig and records the resolved +// option values, dynamic option definitions, and single-machine setting for +// the given provider. +func applyResolvedProviderOptions( + devConfig *config.Config, + providerName string, + resolvedOptionValues map[string]config.OptionValue, + dynamicOptionDefinitions config.OptionDefinitions, + singleMachine *bool, +) *config.Config { + devConfig = config.CloneConfig(devConfig) + if devConfig.Current().Providers == nil { + devConfig.Current().Providers = map[string]*config.ProviderConfig{} + } + if devConfig.Current().Providers[providerName] == nil { + devConfig.Current().Providers[providerName] = &config.ProviderConfig{} + } + + providerCfg := devConfig.Current().Providers[providerName] + providerCfg.Options = map[string]config.OptionValue{} + maps.Copy(providerCfg.Options, resolvedOptionValues) + + providerCfg.DynamicOptions = config.OptionDefinitions{} + maps.Copy(providerCfg.DynamicOptions, dynamicOptionDefinitions) + if singleMachine != nil { + providerCfg.SingleMachine = *singleMachine } - return devConfig, nil + return devConfig } // ResolveAgentConfig resolves and returns the complete agent configuration for a provider. diff --git a/pkg/platform/form/form.go b/pkg/platform/form/form.go index 2a6b86633..d752ca4dd 100644 --- a/pkg/platform/form/form.go +++ b/pkg/platform/form/form.go @@ -31,13 +31,42 @@ func CreateInstance( formCtx, cancelForm := context.WithCancel(ctx) defer cancelForm() + selectedProject, selectedTemplate, selectedTemplateVersion, err := runCreateSelectionForm( + ctx, baseClient, formCtx, cancelForm, + ) + if err != nil { + return nil, err + } + + renderedParameters, err := renderedParametersForCreate( + formCtx, + selectedTemplate, + selectedTemplateVersion, + ) + if err != nil { + return nil, err + } + + return buildCreatedInstance( + id, uid, source, picture, + selectedProject, selectedTemplate, selectedTemplateVersion, + renderedParameters, + ), nil +} + +func runCreateSelectionForm( + ctx context.Context, + baseClient client.Client, + formCtx context.Context, + cancelForm CancelFunc, +) (*managementv1.Project, *managementv1.DevsyWorkspaceTemplate, string, error) { var selectedCluster *managementv1.Cluster var selectedProject *managementv1.Project var selectedTemplate *managementv1.DevsyWorkspaceTemplate selectedTemplateVersion := "" projectOptions, err := projectOptions(ctx, baseClient) if err != nil { - return nil, err + return nil, nil, "", err } err = huh.NewForm( huh.NewGroup( @@ -68,34 +97,37 @@ func CreateInstance( ), ).RunWithContext(formCtx) if err != nil { - return nil, err + return nil, nil, "", err } - parameters := selectedTemplate.Spec.Parameters - if len(selectedTemplate.GetVersions()) > 0 { - parameters, err = list.GetTemplateParameters(selectedTemplate, selectedTemplateVersion) - if err != nil { - return nil, err - } - } - - renderedParameters := "" - if len(parameters) > 0 { - fieldParameters := prepareParameters(parameters) - err = huh.NewForm( - huh.NewGroup(parameterFields(fieldParameters)...), - ).RunWithContext(formCtx) - if err != nil { - return nil, err - } + return selectedProject, selectedTemplate, selectedTemplateVersion, nil +} - renderedParameters, err = renderParameters(fieldParameters) - if err != nil { - return nil, err - } +func renderedParametersForCreate( + formCtx context.Context, + selectedTemplate *managementv1.DevsyWorkspaceTemplate, + selectedTemplateVersion string, +) (string, error) { + parameters, err := resolveTemplateParameters(selectedTemplate, selectedTemplateVersion) + if err != nil { + return "", err + } + if len(parameters) == 0 { + return "", nil } - instance := &managementv1.DevsyWorkspaceInstance{ + fieldParameters := prepareParameters(parameters) + return runParameterForm(formCtx, fieldParameters) +} + +func buildCreatedInstance( + id, uid, source, picture string, + selectedProject *managementv1.Project, + selectedTemplate *managementv1.DevsyWorkspaceTemplate, + selectedTemplateVersion string, + renderedParameters string, +) *managementv1.DevsyWorkspaceInstance { + return &managementv1.DevsyWorkspaceInstance{ ObjectMeta: metav1.ObjectMeta{ GenerateName: encoding.SafeConcatNameMax([]string{id}, 53) + "-", Namespace: project.ProjectNamespace(selectedProject.GetName()), @@ -120,8 +152,32 @@ func CreateInstance( }, }, } +} - return instance, nil +func resolveTemplateParameters( + selectedTemplate *managementv1.DevsyWorkspaceTemplate, + selectedTemplateVersion string, +) ([]storagev1.AppParameter, error) { + parameters := selectedTemplate.Spec.Parameters + if len(selectedTemplate.GetVersions()) > 0 { + var err error + parameters, err = list.GetTemplateParameters(selectedTemplate, selectedTemplateVersion) + if err != nil { + return nil, err + } + } + + return parameters, nil +} + +func runParameterForm(formCtx context.Context, fieldParameters []*FieldParameter) (string, error) { + if err := huh.NewForm( + huh.NewGroup(parameterFields(fieldParameters)...), + ).RunWithContext(formCtx); err != nil { + return "", err + } + + return renderParameters(fieldParameters) } func UpdateInstance( @@ -132,17 +188,55 @@ func UpdateInstance( formCtx, cancelForm := context.WithCancel(ctx) defer cancelForm() + selectedTemplate, selectedTemplateVersion, err := selectUpdateTemplate( + ctx, + baseClient, + formCtx, + instance, + ) + if err != nil { + return nil, err + } + + renderedParameters, err := renderedParametersForUpdate( + formCtx, + instance, + selectedTemplate, + selectedTemplateVersion, + ) + if err != nil { + return nil, err + } + + newInstance := instance.DeepCopy() + applyInstanceChanges(applyInstanceChangesParams{ + instance: instance, + newInstance: newInstance, + selectedTemplate: selectedTemplate, + selectedTemplateVersion: selectedTemplateVersion, + renderedParameters: renderedParameters, + }) + + return newInstance, nil +} + +func selectUpdateTemplate( + ctx context.Context, + baseClient client.Client, + formCtx context.Context, + instance *managementv1.DevsyWorkspaceInstance, +) (*managementv1.DevsyWorkspaceTemplate, string, error) { projectName := project.ProjectFromNamespace(instance.GetNamespace()) projectTemplates, err := list.Templates(ctx, baseClient, projectName) if err != nil { - return nil, err + return nil, "", err } templateOptions, selectedTemplate := templateOptionsForInstance( projectTemplates.DevsyWorkspaceTemplates, instance, ) if selectedTemplate == nil { - return nil, fmt.Errorf("template not found: %#v", instance.Spec.TemplateRef) + return nil, "", fmt.Errorf("template not found: %#v", instance.Spec.TemplateRef) } var selectedTemplateVersion string @@ -166,29 +260,10 @@ func UpdateInstance( ), ).RunWithContext(formCtx) if err != nil { - return nil, err + return nil, "", err } - renderedParameters, err := renderedParametersForUpdate( - formCtx, - instance, - selectedTemplate, - selectedTemplateVersion, - ) - if err != nil { - return nil, err - } - - newInstance := instance.DeepCopy() - applyInstanceChanges(applyInstanceChangesParams{ - instance: instance, - newInstance: newInstance, - selectedTemplate: selectedTemplate, - selectedTemplateVersion: selectedTemplateVersion, - renderedParameters: renderedParameters, - }) - - return newInstance, nil + return selectedTemplate, selectedTemplateVersion, nil } func templateOptionsForInstance( @@ -219,13 +294,9 @@ func renderedParametersForUpdate( selectedTemplate *managementv1.DevsyWorkspaceTemplate, selectedTemplateVersion string, ) (string, error) { - parameters := selectedTemplate.Spec.Parameters - if len(selectedTemplate.GetVersions()) > 0 { - var err error - parameters, err = list.GetTemplateParameters(selectedTemplate, selectedTemplateVersion) - if err != nil { - return "", err - } + parameters, err := resolveTemplateParameters(selectedTemplate, selectedTemplateVersion) + if err != nil { + return "", err } if len(parameters) == 0 { return "", nil @@ -241,14 +312,7 @@ func renderedParametersForUpdate( return "", err } - err = huh.NewForm( - huh.NewGroup(parameterFields(fieldParameters)...), - ).RunWithContext(formCtx) - if err != nil { - return "", err - } - - return renderParameters(fieldParameters) + return runParameterForm(formCtx, fieldParameters) } func buildFieldParameters( diff --git a/pkg/platform/kubeconfig.go b/pkg/platform/kubeconfig.go index 1c8daf446..2050b89d9 100644 --- a/pkg/platform/kubeconfig.go +++ b/pkg/platform/kubeconfig.go @@ -131,75 +131,126 @@ func kubeConfigForSpaceInstance( // direct cluster access? if hostCluster.GetAnnotations()[annotations.LoftDirectClusterEndpoint] != "" { - tok := &managementv1.DirectClusterEndpointToken{ - Spec: managementv1.DirectClusterEndpointTokenSpec{ - Scope: scope, - TTL: ttl, - }, - } - directClusterEndpointToken, err := managementClient.Loft(). - ManagementV1(). - DirectClusterEndpointTokens(). - Create(ctx, tok, metav1.CreateOptions{}) - if err != nil { - return nil, fmt.Errorf("create direct cluster endpoint token: %w", err) - } + return directClusterEndpointKubeConfigForSpace(directClusterEndpointForSpaceParams{ + ctx: ctx, + managementClient: managementClient, + scope: scope, + ttl: ttl, + hostCluster: hostCluster, + projectName: projectName, + spaceInstance: spaceInstance, + }) + } - directClusterEndpoint := hostCluster.GetAnnotations()[annotations.LoftDirectClusterEndpoint] - host := fmt.Sprintf( - "https://%s/kubernetes/project/%s/space/%s", - directClusterEndpoint, - projectName, + // access through management cluster + access key + return kubeConfigViaAccessKey(accessKeyKubeConfigParams{ + ctx: ctx, + baseClient: baseClient, + managementClient: managementClient, + scope: scope, + ttl: ttl, + displayName: fmt.Sprintf( + "Kube Config for Space %s/%s", + spaceInstance.Namespace, spaceInstance.Name, - ) + ), + resourceType: "space", + projectName: projectName, + resourceName: spaceInstance.Name, + clusterRefNamespace: spaceInstance.Spec.ClusterRef.Namespace, + }) +} + +type directClusterEndpointForSpaceParams struct { + ctx context.Context + managementClient kube.Interface + scope *storagev1.AccessKeyScope + ttl int64 + hostCluster managementv1.Cluster + projectName string + spaceInstance *managementv1.SpaceInstance +} - return newKubeConfig( - host, - directClusterEndpointToken.Status.Token, - spaceInstance.Spec.ClusterRef.Namespace, - true, - ), nil +func directClusterEndpointKubeConfigForSpace( + p directClusterEndpointForSpaceParams, +) (*clientcmdapi.Config, error) { + tok := &managementv1.DirectClusterEndpointToken{ + Spec: managementv1.DirectClusterEndpointTokenSpec{ + Scope: p.scope, + TTL: p.ttl, + }, + } + directClusterEndpointToken, err := p.managementClient.Loft(). + ManagementV1(). + DirectClusterEndpointTokens(). + Create(p.ctx, tok, metav1.CreateOptions{}) + if err != nil { + return nil, fmt.Errorf("create direct cluster endpoint token: %w", err) } - // access through management cluster + access key + directClusterEndpoint := p.hostCluster.GetAnnotations()[annotations.LoftDirectClusterEndpoint] + host := fmt.Sprintf( + "https://%s/kubernetes/project/%s/space/%s", + directClusterEndpoint, + p.projectName, + p.spaceInstance.Name, + ) + + return newKubeConfig( + host, + directClusterEndpointToken.Status.Token, + p.spaceInstance.Spec.ClusterRef.Namespace, + true, + ), nil +} + +type accessKeyKubeConfigParams struct { + ctx context.Context + baseClient client.Client + managementClient kube.Interface + scope *storagev1.AccessKeyScope + ttl int64 + displayName string + resourceType string + projectName string + resourceName string + clusterRefNamespace string +} + +// kubeConfigViaAccessKey builds a kube config authenticated via a +// management-cluster-scoped OwnedAccessKey, used when no direct cluster +// endpoint is available. +func kubeConfigViaAccessKey(p accessKeyKubeConfigParams) (*clientcmdapi.Config, error) { key := &managementv1.OwnedAccessKey{ Spec: managementv1.OwnedAccessKeySpec{ AccessKeySpec: storagev1.AccessKeySpec{ - User: baseClient.Self().Status.User.Name, - Scope: scope, - TTL: ttl, - DisplayName: fmt.Sprintf( - "Kube Config for Space %s/%s", - spaceInstance.Namespace, - spaceInstance.Name, - ), + User: p.baseClient.Self().Status.User.Name, + Scope: p.scope, + TTL: p.ttl, + DisplayName: p.displayName, }, }, } - ownedAccessKey, err := managementClient.Loft(). + ownedAccessKey, err := p.managementClient.Loft(). ManagementV1(). OwnedAccessKeys(). - Create(ctx, key, metav1.CreateOptions{}) + Create(p.ctx, key, metav1.CreateOptions{}) if err != nil { return nil, fmt.Errorf("create access key: %w", err) } hostName := strings.TrimPrefix( - strings.TrimPrefix(baseClient.Config().Host, "https://"), + strings.TrimPrefix(p.baseClient.Config().Host, "https://"), "https://", ) host := fmt.Sprintf( - "https://%s/kubernetes/project/%s/space/%s", + "https://%s/kubernetes/project/%s/%s/%s", hostName, - projectName, - spaceInstance.Name, + p.projectName, + p.resourceType, + p.resourceName, ) - return newKubeConfig( - host, - ownedAccessKey.Spec.Key, - spaceInstance.Spec.ClusterRef.Namespace, - true, - ), nil + return newKubeConfig(host, ownedAccessKey.Spec.Key, p.clusterRefNamespace, true), nil } func kubeConfigForVirtualClusterInstance( @@ -222,13 +273,60 @@ func kubeConfigForVirtualClusterInstance( return nil, fmt.Errorf("get virtual cluster instance: %w", err) } + req := newVClusterKubeConfigRequest( + ctx, + managementClient, + namespace, + projectName, + virtualClusterInstance, + ) + + cfg, handled, err := directVirtualClusterKubeConfig( + ctx, + baseClient, + projectName, + virtualClusterInstance, + req, + ) + if handled || err != nil { + return cfg, err + } + + // access through management cluster + access key + return kubeConfigViaAccessKey(accessKeyKubeConfigParams{ + ctx: ctx, + baseClient: baseClient, + managementClient: managementClient, + scope: req.scope, + ttl: req.ttl, + displayName: fmt.Sprintf( + "Kube Config for Virtual Cluster %s/%s", + virtualClusterInstance.Namespace, + virtualClusterInstance.Name, + ), + resourceType: "virtualcluster", + projectName: projectName, + resourceName: virtualClusterInstance.Name, + clusterRefNamespace: virtualClusterInstance.Spec.ClusterRef.Namespace, + }) +} + +// newVClusterKubeConfigRequest builds the scoped request shared by the +// direct-ingress, direct-cluster-endpoint, and access-key kube config paths. +func newVClusterKubeConfigRequest( + ctx context.Context, + managementClient kube.Interface, + namespace, projectName string, + virtualClusterInstance *managementv1.VirtualClusterInstance, +) vClusterKubeConfigRequest { scope := &storagev1.AccessKeyScope{ VirtualClusters: []storagev1.AccessKeyScopeVirtualCluster{{ Project: projectName, VirtualCluster: virtualClusterInstance.Name, }}, } - req := vClusterKubeConfigRequest{ + + return vClusterKubeConfigRequest{ ctx: ctx, managementClient: managementClient, namespace: namespace, @@ -237,11 +335,24 @@ func kubeConfigForVirtualClusterInstance( ttl: int64(configTTL.Seconds()), instance: virtualClusterInstance, } +} +// directVirtualClusterKubeConfig resolves a kube config via direct ingress +// or a direct cluster endpoint, if either is available for this virtual +// cluster. handled reports whether one of those paths applied; if not, the +// caller should fall back to access-key-based config. +func directVirtualClusterKubeConfig( + ctx context.Context, + baseClient client.Client, + projectName string, + virtualClusterInstance *managementv1.VirtualClusterInstance, + req vClusterKubeConfigRequest, +) (cfg *clientcmdapi.Config, handled bool, err error) { // direct virtual cluster ingress access? virtualCluster := virtualClusterInstance.Status.VirtualCluster if virtualCluster != nil && virtualCluster.AccessPoint.Ingress.Enabled { - return directIngressKubeConfig(req) + cfg, err = directIngressKubeConfig(req) + return cfg, true, err } // find cluster by clusterRef @@ -252,53 +363,16 @@ func kubeConfigForVirtualClusterInstance( virtualClusterInstance.Spec.ClusterRef.ClusterRef, ) if err != nil { - return nil, fmt.Errorf("find host cluster: %w", err) + return nil, true, fmt.Errorf("find host cluster: %w", err) } // direct cluster access? if hostCluster.GetAnnotations()[annotations.LoftDirectClusterEndpoint] != "" { - return directClusterEndpointKubeConfig(req, hostCluster) - } - - // access through management cluster + access key - key := &managementv1.OwnedAccessKey{ - Spec: managementv1.OwnedAccessKeySpec{ - AccessKeySpec: storagev1.AccessKeySpec{ - User: baseClient.Self().Status.User.Name, - Scope: scope, - TTL: req.ttl, - DisplayName: fmt.Sprintf( - "Kube Config for Virtual Cluster %s/%s", - virtualClusterInstance.Namespace, - virtualClusterInstance.Name, - ), - }, - }, - } - ownedAccessKey, err := managementClient.Loft(). - ManagementV1(). - OwnedAccessKeys(). - Create(ctx, key, metav1.CreateOptions{}) - if err != nil { - return nil, fmt.Errorf("create access key: %w", err) + cfg, err = directClusterEndpointKubeConfig(req, hostCluster) + return cfg, true, err } - hostName := strings.TrimPrefix( - strings.TrimPrefix(baseClient.Config().Host, "https://"), - "https://", - ) - host := fmt.Sprintf( - "https://%s/kubernetes/project/%s/virtualcluster/%s", - hostName, - projectName, - virtualClusterInstance.Name, - ) - return newKubeConfig( - host, - ownedAccessKey.Spec.Key, - virtualClusterInstance.Spec.ClusterRef.Namespace, - true, - ), nil + return nil, false, nil } type vClusterKubeConfigRequest struct { diff --git a/pkg/ssh/server/ssh.go b/pkg/ssh/server/ssh.go index f7b0eab59..ee26906ee 100644 --- a/pkg/ssh/server/ssh.go +++ b/pkg/ssh/server/ssh.go @@ -171,44 +171,12 @@ func NewServer( return nil, err } - forwardHandler := &ssh.ForwardedTCPHandler{} - forwardedUnixHandler := &ssh.ForwardedUnixHandler{} - keepAliveInterval, keepAliveCountMax := keepAliveConfig() server := &server{ shell: sh, workdir: workdir, reuseSock: reuseSock, currentUser: currentUser.Username, - sshServer: ssh.Server{ - Addr: addr, - ClientAliveInterval: keepAliveInterval, - ClientAliveCountMax: keepAliveCountMax, - LocalPortForwardingCallback: func(ctx ssh.Context, dhost string, dport uint32) bool { - log.Debugf("Accepted forward: %s:%d", dhost, dport) - return true - }, - ReversePortForwardingCallback: func(ctx ssh.Context, host string, port uint32) bool { - log.Debugf("attempt to bind %s:%d - %s", host, port, "granted") - return true - }, - ReverseUnixForwardingCallback: reverseUnixForwardingCallback, - ChannelHandlers: map[string]ssh.ChannelHandler{ - "direct-tcpip": ssh.DirectTCPIPHandler, - "direct-streamlocal@openssh.com": ssh.DirectStreamLocalHandler, - "session": ssh.DefaultSessionHandler, - }, - RequestHandlers: map[string]ssh.RequestHandler{ - "tcpip-forward": forwardHandler.HandleSSHRequest, - "streamlocal-forward@openssh.com": forwardedUnixHandler.HandleSSHRequest, - "cancel-streamlocal-forward@openssh.com": forwardedUnixHandler.HandleSSHRequest, - "cancel-tcpip-forward": forwardHandler.HandleSSHRequest, - }, - SubsystemHandlers: map[string]ssh.SubsystemHandler{ - "sftp": func(s ssh.Session) { - sftpHandler(s, currentUser.Username) - }, - }, - }, + sshServer: buildSSHServer(addr, currentUser.Username), } if len(keys) > 0 { @@ -228,6 +196,45 @@ func NewServer( return server, nil } +// buildSSHServer constructs the underlying ssh.Server with its port +// forwarding, channel, request, and SFTP subsystem handlers wired up. +func buildSSHServer(addr, currentUsername string) ssh.Server { + forwardHandler := &ssh.ForwardedTCPHandler{} + forwardedUnixHandler := &ssh.ForwardedUnixHandler{} + keepAliveInterval, keepAliveCountMax := keepAliveConfig() + + return ssh.Server{ + Addr: addr, + ClientAliveInterval: keepAliveInterval, + ClientAliveCountMax: keepAliveCountMax, + LocalPortForwardingCallback: func(ctx ssh.Context, dhost string, dport uint32) bool { + log.Debugf("Accepted forward: %s:%d", dhost, dport) + return true + }, + ReversePortForwardingCallback: func(ctx ssh.Context, host string, port uint32) bool { + log.Debugf("attempt to bind %s:%d - %s", host, port, "granted") + return true + }, + ReverseUnixForwardingCallback: reverseUnixForwardingCallback, + ChannelHandlers: map[string]ssh.ChannelHandler{ + "direct-tcpip": ssh.DirectTCPIPHandler, + "direct-streamlocal@openssh.com": ssh.DirectStreamLocalHandler, + "session": ssh.DefaultSessionHandler, + }, + RequestHandlers: map[string]ssh.RequestHandler{ + "tcpip-forward": forwardHandler.HandleSSHRequest, + "streamlocal-forward@openssh.com": forwardedUnixHandler.HandleSSHRequest, + "cancel-streamlocal-forward@openssh.com": forwardedUnixHandler.HandleSSHRequest, + "cancel-tcpip-forward": forwardHandler.HandleSSHRequest, + }, + SubsystemHandlers: map[string]ssh.SubsystemHandler{ + "sftp": func(s ssh.Session) { + sftpHandler(s, currentUsername) + }, + }, + } +} + func reverseUnixForwardingCallback(_ ssh.Context, socketPath string) bool { log.Debugf("attempt to bind socket %s", socketPath) From 89b4aa2c1d485e7b1ec398d4ddf78d945d52ef98 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 6 Aug 2026 15:13:30 +0000 Subject: [PATCH 3/5] fix(lint): resolve funlen findings in tests Group C of the funlen backlog: table-driven and multi-scenario tests. Preferred real structural improvement (subtests / separate test functions / data extracted from logic) over cosmetic shrinking. No suppressions were needed -- every flagged test was either a single large data table or a set of independent scenarios, not a genuinely sequential/stateful test that splitting would harm. - pkg/devcontainer/graph/graph_test.go: TestEdgeCount/TestEdgeCases/ TestTopologicalSortAdvanced were table-driven suites where every case was already independent; split each table entry into its own dedicated TestX_ScenarioY suite method (testify's SetupTest runs before each, matching the per-case reset the table loop used to do manually) -- improves failure localization on top of satisfying funlen. - pkg/options/options_test.go: TestInheritFromEnvironment's 4-case table extracted into a package var (inheritFromEnvironmentTestCases); the test itself is now just the t.Run loop. - pkg/options/resolve_test.go: TestResolveOptions (629 lines!) is a single huge data table with a 5-line loop -- extracted the table into a package var (resolveOptionsTestCases) rather than a helper function, since a function returning the same literal would still itself exceed funlen (data length doesn't change by renaming its container). The test itself is now just the range+t.Run loop. - pkg/ssh/config_test.go: TestAddHostSection (313 lines) is the same single-huge-table shape; same treatment -- table extracted into addHostSectionTestCases package var, test body reduced to the s.Run loop. - pkg/types/types_test.go: TestLifecycleHookUnmarshalJSON's 4 JSON-shape scenarios extracted into lifecycleHookUnmarshalTestCases with named lifecycleHookUnmarshalInput/lifecycleHookUnmarshalCase types (previously anonymous structs); test body is now just the t.Run loop. - pkg/workspace/id_test.go: TestToID's 10-case table extracted into toIDTestCases; t.Run subtests were already present, only the table moved out. Every assertion, scenario, and test name preserved exactly. --- pkg/devcontainer/graph/graph_test.go | 322 +++---- pkg/options/options_test.go | 164 ++-- pkg/options/resolve_test.go | 1246 +++++++++++++------------- pkg/ssh/config_test.go | 312 +++---- pkg/types/types_test.go | 34 +- pkg/workspace/id_test.go | 12 +- 6 files changed, 1012 insertions(+), 1078 deletions(-) diff --git a/pkg/devcontainer/graph/graph_test.go b/pkg/devcontainer/graph/graph_test.go index eaedd0e69..0c2768848 100644 --- a/pkg/devcontainer/graph/graph_test.go +++ b/pkg/devcontainer/graph/graph_test.go @@ -231,69 +231,45 @@ func TestGraphSuite(t *testing.T) { suite.Run(t, new(GraphTestSuite)) } -func (suite *GraphTestSuite) TestEdgeCount() { - testCases := []struct { - name string - setup func() - expectedCount int - }{ - { - name: "empty graph", - setup: func() {}, - expectedCount: 0, - }, - { - name: "nodes without edges", - setup: func() { - suite.Require().NoError(suite.graph.AddNode("A", "dataA")) - suite.Require().NoError(suite.graph.AddNode("B", "dataB")) - }, - expectedCount: 0, - }, - { - name: "single edge", - setup: func() { - suite.Require().NoError(suite.graph.AddNode("A", "dataA")) - suite.Require().NoError(suite.graph.AddNode("B", "dataB")) - suite.Require().NoError(suite.graph.AddEdge("A", "B")) - }, - expectedCount: 1, - }, - { - name: "multiple edges", - setup: func() { - suite.Require().NoError(suite.graph.AddNode("A", "dataA")) - suite.Require().NoError(suite.graph.AddNode("B", "dataB")) - suite.Require().NoError(suite.graph.AddNode("C", "dataC")) - suite.Require().NoError(suite.graph.AddEdge("A", "B")) - suite.Require().NoError(suite.graph.AddEdge("A", "C")) - suite.Require().NoError(suite.graph.AddEdge("B", "C")) - }, - expectedCount: 3, - }, - { - name: "edges after removal", - setup: func() { - suite.Require().NoError(suite.graph.AddNode("A", "dataA")) - suite.Require().NoError(suite.graph.AddNode("B", "dataB")) - suite.Require().NoError(suite.graph.AddNode("C", "dataC")) - suite.Require().NoError(suite.graph.AddEdge("A", "B")) - suite.Require().NoError(suite.graph.AddEdge("A", "C")) - suite.Require().NoError(suite.graph.RemoveEdge("A", "B")) - }, - expectedCount: 1, - }, - } +func (suite *GraphTestSuite) TestEdgeCount_EmptyGraph() { + suite.Equal(0, suite.graph.EdgeCount()) +} - for _, tc := range testCases { - suite.Run(tc.name, func() { - suite.SetupTest() - tc.setup() +func (suite *GraphTestSuite) TestEdgeCount_NodesWithoutEdges() { + suite.Require().NoError(suite.graph.AddNode("A", "dataA")) + suite.Require().NoError(suite.graph.AddNode("B", "dataB")) - count := suite.graph.EdgeCount() - suite.Equal(tc.expectedCount, count) - }) - } + suite.Equal(0, suite.graph.EdgeCount()) +} + +func (suite *GraphTestSuite) TestEdgeCount_SingleEdge() { + suite.Require().NoError(suite.graph.AddNode("A", "dataA")) + suite.Require().NoError(suite.graph.AddNode("B", "dataB")) + suite.Require().NoError(suite.graph.AddEdge("A", "B")) + + suite.Equal(1, suite.graph.EdgeCount()) +} + +func (suite *GraphTestSuite) TestEdgeCount_MultipleEdges() { + suite.Require().NoError(suite.graph.AddNode("A", "dataA")) + suite.Require().NoError(suite.graph.AddNode("B", "dataB")) + suite.Require().NoError(suite.graph.AddNode("C", "dataC")) + suite.Require().NoError(suite.graph.AddEdge("A", "B")) + suite.Require().NoError(suite.graph.AddEdge("A", "C")) + suite.Require().NoError(suite.graph.AddEdge("B", "C")) + + suite.Equal(3, suite.graph.EdgeCount()) +} + +func (suite *GraphTestSuite) TestEdgeCount_EdgesAfterRemoval() { + suite.Require().NoError(suite.graph.AddNode("A", "dataA")) + suite.Require().NoError(suite.graph.AddNode("B", "dataB")) + suite.Require().NoError(suite.graph.AddNode("C", "dataC")) + suite.Require().NoError(suite.graph.AddEdge("A", "B")) + suite.Require().NoError(suite.graph.AddEdge("A", "C")) + suite.Require().NoError(suite.graph.RemoveEdge("A", "B")) + + suite.Equal(1, suite.graph.EdgeCount()) } func (suite *GraphTestSuite) TestIsEmpty() { @@ -420,85 +396,58 @@ func (suite *GraphTestSuite) TestGetAllNodes() { suite.Equal("dataB", nodes["B"]) } -func (suite *GraphTestSuite) TestEdgeCases() { - testCases := []struct { - name string - test func() - }{ - { - name: "duplicate edge prevention", - test: func() { - suite.Require().NoError(suite.graph.AddNode("A", "dataA")) - suite.Require().NoError(suite.graph.AddNode("B", "dataB")) - suite.Require().NoError(suite.graph.AddEdge("A", "B")) - suite.Require().NoError(suite.graph.AddEdge("A", "B")) // duplicate - suite.Equal(1, suite.graph.EdgeCount()) - }, - }, - { - name: "remove non-existing node", - test: func() { - err := suite.graph.RemoveNode("missing") - suite.Error(err) - }, - }, - { - name: "get children of non-existing node", - test: func() { - children := suite.graph.GetChildren("missing") - suite.Empty(children) - }, - }, - { - name: "get parents of non-existing node", - test: func() { - parents := suite.graph.GetParents("missing") - suite.Empty(parents) - }, - }, - { - name: "remove edge to non-existing target", - test: func() { - suite.Require().NoError(suite.graph.AddNode("A", "dataA")) - err := suite.graph.RemoveEdge("A", "missing") - suite.Error(err) - }, - }, - { - name: "complex circular dependency", - test: func() { - suite.Require().NoError(suite.graph.AddNode("A", "dataA")) - suite.Require().NoError(suite.graph.AddNode("B", "dataB")) - suite.Require().NoError(suite.graph.AddNode("C", "dataC")) - suite.Require().NoError(suite.graph.AddEdge("A", "B")) - suite.Require().NoError(suite.graph.AddEdge("B", "C")) - suite.Require().NoError(suite.graph.AddEdge("C", "A")) // creates cycle - suite.True(suite.graph.HasCircularDependency()) - }, - }, - { - name: "self loop", - test: func() { - suite.Require().NoError(suite.graph.AddNode("A", "dataA")) - suite.Require().NoError(suite.graph.AddEdge("A", "A")) - suite.True(suite.graph.HasCircularDependency()) - }, - }, - { - name: "remove subgraph of non-existing node", - test: func() { - err := suite.graph.RemoveSubGraph("missing") - suite.NoError(err) // should not error - }, - }, - } +func (suite *GraphTestSuite) TestEdgeCases_DuplicateEdgePrevention() { + suite.Require().NoError(suite.graph.AddNode("A", "dataA")) + suite.Require().NoError(suite.graph.AddNode("B", "dataB")) + suite.Require().NoError(suite.graph.AddEdge("A", "B")) + suite.Require().NoError(suite.graph.AddEdge("A", "B")) // duplicate - for _, tc := range testCases { - suite.Run(tc.name, func() { - suite.SetupTest() - tc.test() - }) - } + suite.Equal(1, suite.graph.EdgeCount()) +} + +func (suite *GraphTestSuite) TestEdgeCases_RemoveNonExistingNode() { + err := suite.graph.RemoveNode("missing") + suite.Error(err) +} + +func (suite *GraphTestSuite) TestEdgeCases_GetChildrenOfNonExistingNode() { + children := suite.graph.GetChildren("missing") + suite.Empty(children) +} + +func (suite *GraphTestSuite) TestEdgeCases_GetParentsOfNonExistingNode() { + parents := suite.graph.GetParents("missing") + suite.Empty(parents) +} + +func (suite *GraphTestSuite) TestEdgeCases_RemoveEdgeToNonExistingTarget() { + suite.Require().NoError(suite.graph.AddNode("A", "dataA")) + + err := suite.graph.RemoveEdge("A", "missing") + suite.Error(err) +} + +func (suite *GraphTestSuite) TestEdgeCases_ComplexCircularDependency() { + suite.Require().NoError(suite.graph.AddNode("A", "dataA")) + suite.Require().NoError(suite.graph.AddNode("B", "dataB")) + suite.Require().NoError(suite.graph.AddNode("C", "dataC")) + suite.Require().NoError(suite.graph.AddEdge("A", "B")) + suite.Require().NoError(suite.graph.AddEdge("B", "C")) + suite.Require().NoError(suite.graph.AddEdge("C", "A")) // creates cycle + + suite.True(suite.graph.HasCircularDependency()) +} + +func (suite *GraphTestSuite) TestEdgeCases_SelfLoop() { + suite.Require().NoError(suite.graph.AddNode("A", "dataA")) + suite.Require().NoError(suite.graph.AddEdge("A", "A")) + + suite.True(suite.graph.HasCircularDependency()) +} + +func (suite *GraphTestSuite) TestEdgeCases_RemoveSubGraphOfNonExistingNode() { + err := suite.graph.RemoveSubGraph("missing") + suite.NoError(err) // should not error } func (suite *GraphTestSuite) TestIsReachable() { @@ -575,71 +524,40 @@ func (suite *GraphTestSuite) TestSortNodeIDsRoundBasedMultiLevel() { suite.Equal([]string{"A", "C", "B", "D"}, result) } -func (suite *GraphTestSuite) TestTopologicalSortAdvanced() { - testCases := []struct { - name string - setup func() - wantErr bool - verify func([]string) - }{ - { - name: "empty graph", - setup: func() {}, - wantErr: false, - verify: func(result []string) { - suite.Empty(result) - }, - }, - { - name: "diamond dependency", - setup: func() { - suite.Require().NoError(suite.graph.AddNode("A", "dataA")) - suite.Require().NoError(suite.graph.AddNode("B", "dataB")) - suite.Require().NoError(suite.graph.AddNode("C", "dataC")) - suite.Require().NoError(suite.graph.AddNode("D", "dataD")) - suite.Require().NoError(suite.graph.AddEdge("A", "B")) - suite.Require().NoError(suite.graph.AddEdge("A", "C")) - suite.Require().NoError(suite.graph.AddEdge("B", "D")) - suite.Require().NoError(suite.graph.AddEdge("C", "D")) - }, - wantErr: false, - verify: func(result []string) { - suite.Len(result, 4) - suite.Equal("dataA", result[0]) - suite.Equal("dataD", result[3]) - }, - }, - { - name: "three node cycle", - setup: func() { - suite.Require().NoError(suite.graph.AddNode("A", "dataA")) - suite.Require().NoError(suite.graph.AddNode("B", "dataB")) - suite.Require().NoError(suite.graph.AddNode("C", "dataC")) - suite.Require().NoError(suite.graph.AddEdge("A", "B")) - suite.Require().NoError(suite.graph.AddEdge("B", "C")) - suite.Require().NoError(suite.graph.AddEdge("C", "A")) - }, - wantErr: true, - verify: func(result []string) { - suite.Nil(result) - }, - }, - } +func (suite *GraphTestSuite) TestTopologicalSortAdvanced_EmptyGraph() { + result, err := suite.graph.Sort() + suite.NoError(err) + suite.Empty(result) +} - for _, tc := range testCases { - suite.Run(tc.name, func() { - suite.SetupTest() - tc.setup() +func (suite *GraphTestSuite) TestTopologicalSortAdvanced_DiamondDependency() { + suite.Require().NoError(suite.graph.AddNode("A", "dataA")) + suite.Require().NoError(suite.graph.AddNode("B", "dataB")) + suite.Require().NoError(suite.graph.AddNode("C", "dataC")) + suite.Require().NoError(suite.graph.AddNode("D", "dataD")) + suite.Require().NoError(suite.graph.AddEdge("A", "B")) + suite.Require().NoError(suite.graph.AddEdge("A", "C")) + suite.Require().NoError(suite.graph.AddEdge("B", "D")) + suite.Require().NoError(suite.graph.AddEdge("C", "D")) - result, err := suite.graph.Sort() - if tc.wantErr { - suite.Error(err) - } else { - suite.NoError(err) - } - tc.verify(result) - }) - } + result, err := suite.graph.Sort() + suite.NoError(err) + suite.Len(result, 4) + suite.Equal("dataA", result[0]) + suite.Equal("dataD", result[3]) +} + +func (suite *GraphTestSuite) TestTopologicalSortAdvanced_ThreeNodeCycle() { + suite.Require().NoError(suite.graph.AddNode("A", "dataA")) + suite.Require().NoError(suite.graph.AddNode("B", "dataB")) + suite.Require().NoError(suite.graph.AddNode("C", "dataC")) + suite.Require().NoError(suite.graph.AddEdge("A", "B")) + suite.Require().NoError(suite.graph.AddEdge("B", "C")) + suite.Require().NoError(suite.graph.AddEdge("C", "A")) + + result, err := suite.graph.Sort() + suite.Error(err) + suite.Nil(result) } func (suite *GraphTestSuite) TestSortWithPriorityRoundBased() { diff --git a/pkg/options/options_test.go b/pkg/options/options_test.go index b75073283..e9c857676 100644 --- a/pkg/options/options_test.go +++ b/pkg/options/options_test.go @@ -20,94 +20,100 @@ type assignmentTestCase struct { ExpectedAssignments []string } -func TestInheritFromEnvironment(t *testing.T) { - testCases := []assignmentTestCase{ - { - Name: "assigned, not in the environment", - Names: []string{ - "HOST", - }, - Assignments: []string{ - "HOST=box", - }, - EnvironmentVariablePrefix: "DEVSY_PROVIDER_SSH_", - NotInEnvironment: []string{ - "DEVSY_PROVIDER_SSH_HOST", - }, - Environment: map[string]string{}, - ExpectedAssignments: []string{ - "HOST=box", - }, +var inheritFromEnvironmentTestCases = []assignmentTestCase{ + { + Name: "assigned, not in the environment", + Names: []string{ + "HOST", + }, + Assignments: []string{ + "HOST=box", + }, + EnvironmentVariablePrefix: "DEVSY_PROVIDER_SSH_", + NotInEnvironment: []string{ + "DEVSY_PROVIDER_SSH_HOST", + }, + Environment: map[string]string{}, + ExpectedAssignments: []string{ + "HOST=box", + }, + }, + { + Name: "not assigned, not in the environment", + Names: []string{ + "HOST", + }, + Assignments: []string{}, + EnvironmentVariablePrefix: "DEVSY_PROVIDER_SSH_", + NotInEnvironment: []string{ + "DEVSY_PROVIDER_SSH_HOST", + }, + Environment: map[string]string{}, + ExpectedAssignments: []string{}, + }, + { + Name: "assigned, in the environment", + Names: []string{ + "HOST", + }, + Assignments: []string{ + "HOST=box", + }, + EnvironmentVariablePrefix: "DEVSY_PROVIDER_SSH_", + NotInEnvironment: []string{}, + Environment: map[string]string{ + "DEVSY_PROVIDER_SSH_HOST": "another-box", }, - { - Name: "not assigned, not in the environment", - Names: []string{ - "HOST", - }, - Assignments: []string{}, - EnvironmentVariablePrefix: "DEVSY_PROVIDER_SSH_", - NotInEnvironment: []string{ - "DEVSY_PROVIDER_SSH_HOST", - }, - Environment: map[string]string{}, - ExpectedAssignments: []string{}, + ExpectedAssignments: []string{ + "HOST=box", }, - { - Name: "assigned, in the environment", - Names: []string{ - "HOST", - }, - Assignments: []string{ - "HOST=box", - }, - EnvironmentVariablePrefix: "DEVSY_PROVIDER_SSH_", - NotInEnvironment: []string{}, - Environment: map[string]string{ - "DEVSY_PROVIDER_SSH_HOST": "another-box", - }, - ExpectedAssignments: []string{ - "HOST=box", - }, + }, + { + Name: "not assigned, in the environment", + Names: []string{ + "HOST", }, - { - Name: "not assigned, in the environment", - Names: []string{ - "HOST", - }, - Assignments: []string{}, - EnvironmentVariablePrefix: "DEVSY_PROVIDER_SSH_", - NotInEnvironment: []string{}, - Environment: map[string]string{ - "DEVSY_PROVIDER_SSH_HOST": "another-box", - }, - ExpectedAssignments: []string{ - "HOST=another-box", - }, + Assignments: []string{}, + EnvironmentVariablePrefix: "DEVSY_PROVIDER_SSH_", + NotInEnvironment: []string{}, + Environment: map[string]string{ + "DEVSY_PROVIDER_SSH_HOST": "another-box", }, + ExpectedAssignments: []string{ + "HOST=another-box", + }, + }, +} + +func TestInheritFromEnvironment(t *testing.T) { + for _, testCase := range inheritFromEnvironmentTestCases { + t.Run(testCase.Name, func(t *testing.T) { + runInheritFromEnvironmentTestCase(t, testCase) + }) } +} - for _, testCase := range testCases { - fmt.Println(testCase.Name) +func runInheritFromEnvironmentTestCase(t *testing.T, testCase assignmentTestCase) { + fmt.Println(testCase.Name) - for _, k := range testCase.NotInEnvironment { - err := os.Unsetenv(k) - if err != nil { - t.Fatalf("unexpected error %v in %s", err, testCase.Name) - } + for _, k := range testCase.NotInEnvironment { + err := os.Unsetenv(k) + if err != nil { + t.Fatalf("unexpected error %v in %s", err, testCase.Name) } - for k, v := range testCase.Environment { - err := os.Setenv(k, v) - if err != nil { - t.Fatalf("unexpected error %v in %s", err, testCase.Name) - } + } + for k, v := range testCase.Environment { + err := os.Setenv(k, v) + if err != nil { + t.Fatalf("unexpected error %v in %s", err, testCase.Name) } + } - result := InheritFromEnvironment( - testCase.Assignments, - testCase.Names, - testCase.EnvironmentVariablePrefix, - ) + result := InheritFromEnvironment( + testCase.Assignments, + testCase.Names, + testCase.EnvironmentVariablePrefix, + ) - assert.DeepEqual(t, result, testCase.ExpectedAssignments) - } + assert.DeepEqual(t, result, testCase.ExpectedAssignments) } diff --git a/pkg/options/resolve_test.go b/pkg/options/resolve_test.go index aeecfa647..18d7f1a94 100644 --- a/pkg/options/resolve_test.go +++ b/pkg/options/resolve_test.go @@ -31,632 +31,632 @@ type testCase struct { ExpectedDynamicOptions config.OptionDefinitions } -func TestResolveOptions(t *testing.T) { - testCases := []testCase{ - { - Name: "simple", - ExtraValues: map[string]string{ - "WORKSPACE_ID": "test", - }, - ProviderOptions: map[string]*types.Option{ - "TEST": { - Default: "${WORKSPACE_ID}-test", - }, - }, - ExpectedOptions: map[string]string{ - "TEST": "test-test", - }, - }, - { - Name: "dependency", - ExtraValues: map[string]string{ - "WORKSPACE_ID": "test", - }, - ProviderOptions: map[string]*types.Option{ - "TEST": { - Default: "${WORKSPACE_ID}-test-${COMMAND}-$COMMAND", - }, - "COMMAND": { - Command: "echo bar", - }, - }, - ExpectedOptions: map[string]string{ - "TEST": "test-test-bar-bar", - "COMMAND": "bar", - }, - }, - { - Name: "No extra values", - ProviderOptions: map[string]*types.Option{ - "COMMAND1": { - Command: "echo ${COMMAND2}-test", - }, - "COMMAND2": { - Command: "echo bar", - }, - }, - ExpectedOptions: map[string]string{ - "COMMAND1": "bar-test", - "COMMAND2": "bar", - }, - }, - { - Name: "Cyclic dep", - ProviderOptions: map[string]*types.Option{ - "COMMAND1": { - Command: "echo ${COMMAND2}", - }, - "COMMAND2": { - Command: "echo ${COMMAND1}", - }, - }, - ExpectErr: true, - }, - { - Name: "Override", - ResolvedValues: map[string]config.OptionValue{ - "COMMAND": { - Value: "foo", - UserProvided: true, - }, - }, - ProviderOptions: map[string]*types.Option{ - "COMMAND": { - Command: "echo bar", - }, - }, - ExpectedOptions: map[string]string{ - "COMMAND": "foo", - }, - }, - { - Name: "Override", - ResolvedValues: map[string]config.OptionValue{ - "COMMAND": { - Value: "foo", - UserProvided: true, - }, - }, - ProviderOptions: map[string]*types.Option{ - "COMMAND": { - Command: "echo bar", - }, - "COMMAND1": { - Command: "echo ${COMMAND}-foo-${UNDEFINED}", - }, - "DEFAULT1": { - Default: "${COMMAND}-foo-${UNDEFINED}", - }, - }, - ExpectedOptions: map[string]string{ - "COMMAND": "foo", - "COMMAND1": "foo-foo-", - "DEFAULT1": "foo-foo-${UNDEFINED}", - }, - }, - { - Name: "Expire", - ResolvedValues: map[string]config.OptionValue{ - "EXPIRE": { - Value: "foo", - Filled: &[]types.Time{types.NewTime(time.Time{})}[0], - }, - "NOTEXPIRE": { - Value: "foo", - Filled: &[]types.Time{types.Now()}[0], - }, - }, - ProviderOptions: map[string]*types.Option{ - "EXPIRE": { - Command: "echo bar", - Cache: "10m", - }, - "NOTEXPIRE": { - Command: "echo bar", - Cache: "10m", - }, - }, - ExpectedOptions: map[string]string{ - "EXPIRE": "bar", - "NOTEXPIRE": "foo", - }, - }, - { - Name: "Ignore self", - ProviderOptions: map[string]*types.Option{ - "SELF": { - Command: "SELF=test; echo ${SELF}", - }, - }, - ExpectedOptions: map[string]string{ - "SELF": "test", - }, - }, - { - Name: "Recompute children", - UserValues: map[string]string{ - "PARENT": "foo", - }, - ResolvedValues: map[string]config.OptionValue{ - "PARENT": { - Value: "test", - UserProvided: true, - }, - "CHILD1": { - Value: "test-child1", - }, - "CHILD2": { - Value: "test-child2", - }, - }, - ProviderOptions: map[string]*types.Option{ - "PARENT": {}, - "CHILD1": { - Command: "echo ${PARENT}-child1", - }, - "CHILD2": { - Default: "${PARENT}-child2", - }, - }, - ExpectedOptions: map[string]string{ - "PARENT": "foo", - "CHILD1": "foo-child1", - "CHILD2": "foo-child2", - }, - }, - { - Name: "Error local global", - ProviderOptions: map[string]*types.Option{ - "PARENT": { - Default: "test", - }, - "CHILD1": { - Global: true, - Default: "${PARENT}", - }, - }, - ExpectErr: true, - }, - { - Name: "Error local var", - ProviderOptions: map[string]*types.Option{ - "PARENT": { - Local: true, - Default: "test", - }, - "CHILD1": { - Default: "${PARENT}", - }, - }, - ExpectErr: true, - }, - { - Name: "Don't resolve local", - ProviderOptions: map[string]*types.Option{ - "PARENT": { - Default: "test", - }, - "CHILD1": { - Default: "${PARENT}", - Local: true, - }, - }, - DontResolveLocal: true, - ExpectedOptions: map[string]string{ - "PARENT": "test", - }, - }, - { - Name: "Resolve", - ProviderOptions: map[string]*types.Option{ - "PARENT": { - Default: "test", - }, - "CHILD1": { - Default: "${PARENT}", - }, - }, - DontResolveLocal: true, - ExpectedOptions: map[string]string{ - "PARENT": "test", - "CHILD1": "test", - }, - }, - { - Name: "Skip Required", - ProviderOptions: map[string]*types.Option{ - "PARENT": { - Required: true, - }, - "CHILD1": { - Default: "${PARENT}", - }, - "PARENT2": { - Required: true, - Default: "test", - }, - "CHILD2": { - Default: "${PARENT2}", - }, - }, - SkipRequired: true, - ExpectedOptions: map[string]string{ - "PARENT2": "test", - "CHILD2": "test", - }, - }, - { - Name: "Nested dynamic options", - ProviderOptions: map[string]*types.Option{ - "TEST": { - Default: "test", - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST2": &types.Option{ - Default: "test2", - }, - }), - }, - "FOO": {Command: "echo bar"}, - }, - ExpectedOptions: map[string]string{ - "TEST": "test", - "TEST2": "test2", - "FOO": "bar", - }, - ExpectedDynamicOptions: config.OptionDefinitions{ - "TEST2": &types.Option{ - Default: "test2", - }, - }, - }, - { - Name: "Dynamic options don't update", - ProviderOptions: map[string]*types.Option{ - "TEST": { - Default: "test", - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST2": &types.Option{ - Default: "test2", - }, - }), - }, - "FOO": {Command: "echo bar"}, - }, - ResolvedDynamicDefinitions: map[string]*types.Option{ - "TEST2": { - Default: "test5", - }, - }, - ResolvedValues: map[string]config.OptionValue{ - "TEST": {Value: "test3", Children: []string{"TEST2"}, UserProvided: true}, - "TEST2": {Value: "test4", UserProvided: true}, - }, - ExpectedOptions: map[string]string{ - "TEST": "test3", - "TEST2": "test4", - "FOO": "bar", - }, - ExpectedDynamicOptions: config.OptionDefinitions{ - "TEST2": { - Default: "test2", - }, - }, - }, - { - Name: "Dynamic options update", - ProviderOptions: map[string]*types.Option{ - "TEST": { - Default: "test", - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST3": &types.Option{ - Default: "test2", - }, - }), - }, - "FOO": {Command: "echo bar"}, - }, - UserValues: map[string]string{ - "TEST": "test1", - }, - ResolvedValues: map[string]config.OptionValue{ - "TEST": {Value: "test3", Children: []string{"TEST2"}}, - "TEST2": {Value: "test4"}, - }, - ResolvedDynamicDefinitions: map[string]*types.Option{ - "TEST2": { - Default: "test5", - }, - }, - ExpectedOptions: map[string]string{ - "TEST": "test1", - "TEST3": "test2", - "FOO": "bar", - }, - ExpectedDynamicOptions: config.OptionDefinitions{ - "TEST3": &types.Option{ - Default: "test2", - }, - }, - }, - { - Name: "Nested dynamic options", - ProviderOptions: map[string]*types.Option{ - "TEST": { - Default: "test1", - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST2": &types.Option{ - Default: "test2", - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST3": &types.Option{ - Default: "test3", - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST4": &types.Option{ - Default: "${TEST3}-${FOO}-4", - }, - }), - }, - }), - }, - }), - }, - "FOO": {Command: "echo bar"}, - }, - ExpectedOptions: map[string]string{ - "TEST": "test1", - "TEST2": "test2", - "TEST3": "test3", - "TEST4": "test3-bar-4", - "FOO": "bar", - }, - ExpectedDynamicOptions: config.OptionDefinitions{ - "TEST2": &types.Option{ - Default: "test2", - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST3": &types.Option{ - Default: "test3", - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST4": &types.Option{ - Default: "${TEST3}-${FOO}-4", - }, - }), - }, - }), - }, - "TEST3": &types.Option{ - Default: "test3", - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST4": &types.Option{ - Default: "${TEST3}-${FOO}-4", - }, - }), - }, - "TEST4": &types.Option{ - Default: "${TEST3}-${FOO}-4", - }, - }, - }, - { - Name: "Nested dynamic options skip required", - ProviderOptions: map[string]*types.Option{ - "TEST": { - Default: "test1", - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST2": &types.Option{ - Required: true, - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST3": &types.Option{ - Default: "test3", - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST4": &types.Option{ - Default: "${TEST3}-${FOO}-4", - }, - }), - }, - }), - }, - }), - }, - "FOO": {Command: "echo bar"}, - }, - SkipRequired: true, - ExpectedOptions: map[string]string{ - "TEST": "test1", - "FOO": "bar", - }, - ExpectedDynamicOptions: config.OptionDefinitions{ - "TEST2": &types.Option{ - Required: true, - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST3": &types.Option{ - Default: "test3", - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST4": &types.Option{ - Default: "${TEST3}-${FOO}-4", - }, - }), - }, - }), - }, - }, - }, - { - Name: "Nested dynamic options use option", - ProviderOptions: map[string]*types.Option{ - "TEST": { - Default: "test1", - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST2": &types.Option{ - Required: true, - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST3": &types.Option{ - Default: "test3", - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST4": &types.Option{ - Default: "${TEST2}-${FOO}-4", - }, - }), - }, - }), - }, - }), - }, - "FOO": {Command: "echo bar"}, - }, - SkipRequired: true, - UserValues: map[string]string{ - "TEST2": "test2", - }, - ExpectedOptions: map[string]string{ - "TEST": "test1", - "TEST2": "test2", - "TEST3": "test3", - "TEST4": "test2-bar-4", - "FOO": "bar", - }, - ExpectedDynamicOptions: config.OptionDefinitions{ - "TEST2": &types.Option{ - Required: true, - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST3": &types.Option{ - Default: "test3", - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST4": &types.Option{ - Default: "${TEST2}-${FOO}-4", - }, - }), - }, - }), - }, - "TEST3": &types.Option{ - Default: "test3", - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST4": &types.Option{ - Default: "${TEST2}-${FOO}-4", - }, - }), - }, - "TEST4": &types.Option{ - Default: "${TEST2}-${FOO}-4", - }, - }, - }, - { - Name: "Nested dynamic options use option", - ProviderOptions: map[string]*types.Option{ - "TEST": { - Default: "test1", - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST2": &types.Option{ - Default: "test2", - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST3": &types.Option{ - Default: "test3", - }, - }), - }, - }), - }, - "FOO": {Command: "echo bar"}, - }, - ResolvedValues: map[string]config.OptionValue{ - "TEST5": { - Value: "test5", - }, - }, - ExpectedOptions: map[string]string{ - "TEST": "test1", - "TEST2": "test2", - "TEST3": "test3", - "FOO": "bar", - }, - ExpectedDynamicOptions: config.OptionDefinitions{ - "TEST2": &types.Option{ - Default: "test2", - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST3": &types.Option{ - Default: "test3", - }, - }), - }, - "TEST3": &types.Option{ - Default: "test3", - }, - }, - }, - { - Name: "Dynamic options unused option", - ProviderOptions: map[string]*types.Option{ - "TEST": { - Default: "test1", - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST2": &types.Option{ - Default: "test2", - }, - }), - }, - "FOO": {Command: "echo bar"}, - }, - ResolvedValues: map[string]config.OptionValue{ - "TEST5": { - Value: "test5", - }, - }, - ResolvedDynamicDefinitions: map[string]*types.Option{ - "TEST5": { - Default: "test2", - }, - }, - ExpectedOptions: map[string]string{ - "TEST": "test1", - "TEST2": "test2", - "FOO": "bar", - }, - ExpectedDynamicOptions: config.OptionDefinitions{ - "TEST2": &types.Option{ - Default: "test2", - }, - }, - }, - { - Name: "Dynamic options update default", - ProviderOptions: map[string]*types.Option{ - "TEST": { - Default: "test1", - SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST2": &types.Option{ - Default: "test3", - }, - }), - }, - "FOO": {Command: "echo bar"}, - }, - ResolvedValues: map[string]config.OptionValue{ - "TEST": { - Value: "test1", - }, - "TEST2": { - Value: "test2", - }, - }, - ResolvedDynamicDefinitions: map[string]*types.Option{ - "TEST2": { - Default: "test2", - }, - }, - ExpectedOptions: map[string]string{ - "TEST": "test1", - "TEST2": "test3", - "FOO": "bar", - }, - ExpectedDynamicOptions: config.OptionDefinitions{ - "TEST2": &types.Option{ - Default: "test3", - }, +var resolveOptionsTestCases = []testCase{ + { + Name: "simple", + ExtraValues: map[string]string{ + "WORKSPACE_ID": "test", + }, + ProviderOptions: map[string]*types.Option{ + "TEST": { + Default: "${WORKSPACE_ID}-test", }, }, - } + ExpectedOptions: map[string]string{ + "TEST": "test-test", + }, + }, + { + Name: "dependency", + ExtraValues: map[string]string{ + "WORKSPACE_ID": "test", + }, + ProviderOptions: map[string]*types.Option{ + "TEST": { + Default: "${WORKSPACE_ID}-test-${COMMAND}-$COMMAND", + }, + "COMMAND": { + Command: "echo bar", + }, + }, + ExpectedOptions: map[string]string{ + "TEST": "test-test-bar-bar", + "COMMAND": "bar", + }, + }, + { + Name: "No extra values", + ProviderOptions: map[string]*types.Option{ + "COMMAND1": { + Command: "echo ${COMMAND2}-test", + }, + "COMMAND2": { + Command: "echo bar", + }, + }, + ExpectedOptions: map[string]string{ + "COMMAND1": "bar-test", + "COMMAND2": "bar", + }, + }, + { + Name: "Cyclic dep", + ProviderOptions: map[string]*types.Option{ + "COMMAND1": { + Command: "echo ${COMMAND2}", + }, + "COMMAND2": { + Command: "echo ${COMMAND1}", + }, + }, + ExpectErr: true, + }, + { + Name: "Override", + ResolvedValues: map[string]config.OptionValue{ + "COMMAND": { + Value: "foo", + UserProvided: true, + }, + }, + ProviderOptions: map[string]*types.Option{ + "COMMAND": { + Command: "echo bar", + }, + }, + ExpectedOptions: map[string]string{ + "COMMAND": "foo", + }, + }, + { + Name: "Override", + ResolvedValues: map[string]config.OptionValue{ + "COMMAND": { + Value: "foo", + UserProvided: true, + }, + }, + ProviderOptions: map[string]*types.Option{ + "COMMAND": { + Command: "echo bar", + }, + "COMMAND1": { + Command: "echo ${COMMAND}-foo-${UNDEFINED}", + }, + "DEFAULT1": { + Default: "${COMMAND}-foo-${UNDEFINED}", + }, + }, + ExpectedOptions: map[string]string{ + "COMMAND": "foo", + "COMMAND1": "foo-foo-", + "DEFAULT1": "foo-foo-${UNDEFINED}", + }, + }, + { + Name: "Expire", + ResolvedValues: map[string]config.OptionValue{ + "EXPIRE": { + Value: "foo", + Filled: &[]types.Time{types.NewTime(time.Time{})}[0], + }, + "NOTEXPIRE": { + Value: "foo", + Filled: &[]types.Time{types.Now()}[0], + }, + }, + ProviderOptions: map[string]*types.Option{ + "EXPIRE": { + Command: "echo bar", + Cache: "10m", + }, + "NOTEXPIRE": { + Command: "echo bar", + Cache: "10m", + }, + }, + ExpectedOptions: map[string]string{ + "EXPIRE": "bar", + "NOTEXPIRE": "foo", + }, + }, + { + Name: "Ignore self", + ProviderOptions: map[string]*types.Option{ + "SELF": { + Command: "SELF=test; echo ${SELF}", + }, + }, + ExpectedOptions: map[string]string{ + "SELF": "test", + }, + }, + { + Name: "Recompute children", + UserValues: map[string]string{ + "PARENT": "foo", + }, + ResolvedValues: map[string]config.OptionValue{ + "PARENT": { + Value: "test", + UserProvided: true, + }, + "CHILD1": { + Value: "test-child1", + }, + "CHILD2": { + Value: "test-child2", + }, + }, + ProviderOptions: map[string]*types.Option{ + "PARENT": {}, + "CHILD1": { + Command: "echo ${PARENT}-child1", + }, + "CHILD2": { + Default: "${PARENT}-child2", + }, + }, + ExpectedOptions: map[string]string{ + "PARENT": "foo", + "CHILD1": "foo-child1", + "CHILD2": "foo-child2", + }, + }, + { + Name: "Error local global", + ProviderOptions: map[string]*types.Option{ + "PARENT": { + Default: "test", + }, + "CHILD1": { + Global: true, + Default: "${PARENT}", + }, + }, + ExpectErr: true, + }, + { + Name: "Error local var", + ProviderOptions: map[string]*types.Option{ + "PARENT": { + Local: true, + Default: "test", + }, + "CHILD1": { + Default: "${PARENT}", + }, + }, + ExpectErr: true, + }, + { + Name: "Don't resolve local", + ProviderOptions: map[string]*types.Option{ + "PARENT": { + Default: "test", + }, + "CHILD1": { + Default: "${PARENT}", + Local: true, + }, + }, + DontResolveLocal: true, + ExpectedOptions: map[string]string{ + "PARENT": "test", + }, + }, + { + Name: "Resolve", + ProviderOptions: map[string]*types.Option{ + "PARENT": { + Default: "test", + }, + "CHILD1": { + Default: "${PARENT}", + }, + }, + DontResolveLocal: true, + ExpectedOptions: map[string]string{ + "PARENT": "test", + "CHILD1": "test", + }, + }, + { + Name: "Skip Required", + ProviderOptions: map[string]*types.Option{ + "PARENT": { + Required: true, + }, + "CHILD1": { + Default: "${PARENT}", + }, + "PARENT2": { + Required: true, + Default: "test", + }, + "CHILD2": { + Default: "${PARENT2}", + }, + }, + SkipRequired: true, + ExpectedOptions: map[string]string{ + "PARENT2": "test", + "CHILD2": "test", + }, + }, + { + Name: "Nested dynamic options", + ProviderOptions: map[string]*types.Option{ + "TEST": { + Default: "test", + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST2": &types.Option{ + Default: "test2", + }, + }), + }, + "FOO": {Command: "echo bar"}, + }, + ExpectedOptions: map[string]string{ + "TEST": "test", + "TEST2": "test2", + "FOO": "bar", + }, + ExpectedDynamicOptions: config.OptionDefinitions{ + "TEST2": &types.Option{ + Default: "test2", + }, + }, + }, + { + Name: "Dynamic options don't update", + ProviderOptions: map[string]*types.Option{ + "TEST": { + Default: "test", + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST2": &types.Option{ + Default: "test2", + }, + }), + }, + "FOO": {Command: "echo bar"}, + }, + ResolvedDynamicDefinitions: map[string]*types.Option{ + "TEST2": { + Default: "test5", + }, + }, + ResolvedValues: map[string]config.OptionValue{ + "TEST": {Value: "test3", Children: []string{"TEST2"}, UserProvided: true}, + "TEST2": {Value: "test4", UserProvided: true}, + }, + ExpectedOptions: map[string]string{ + "TEST": "test3", + "TEST2": "test4", + "FOO": "bar", + }, + ExpectedDynamicOptions: config.OptionDefinitions{ + "TEST2": { + Default: "test2", + }, + }, + }, + { + Name: "Dynamic options update", + ProviderOptions: map[string]*types.Option{ + "TEST": { + Default: "test", + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST3": &types.Option{ + Default: "test2", + }, + }), + }, + "FOO": {Command: "echo bar"}, + }, + UserValues: map[string]string{ + "TEST": "test1", + }, + ResolvedValues: map[string]config.OptionValue{ + "TEST": {Value: "test3", Children: []string{"TEST2"}}, + "TEST2": {Value: "test4"}, + }, + ResolvedDynamicDefinitions: map[string]*types.Option{ + "TEST2": { + Default: "test5", + }, + }, + ExpectedOptions: map[string]string{ + "TEST": "test1", + "TEST3": "test2", + "FOO": "bar", + }, + ExpectedDynamicOptions: config.OptionDefinitions{ + "TEST3": &types.Option{ + Default: "test2", + }, + }, + }, + { + Name: "Nested dynamic options", + ProviderOptions: map[string]*types.Option{ + "TEST": { + Default: "test1", + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST2": &types.Option{ + Default: "test2", + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST3": &types.Option{ + Default: "test3", + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST4": &types.Option{ + Default: "${TEST3}-${FOO}-4", + }, + }), + }, + }), + }, + }), + }, + "FOO": {Command: "echo bar"}, + }, + ExpectedOptions: map[string]string{ + "TEST": "test1", + "TEST2": "test2", + "TEST3": "test3", + "TEST4": "test3-bar-4", + "FOO": "bar", + }, + ExpectedDynamicOptions: config.OptionDefinitions{ + "TEST2": &types.Option{ + Default: "test2", + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST3": &types.Option{ + Default: "test3", + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST4": &types.Option{ + Default: "${TEST3}-${FOO}-4", + }, + }), + }, + }), + }, + "TEST3": &types.Option{ + Default: "test3", + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST4": &types.Option{ + Default: "${TEST3}-${FOO}-4", + }, + }), + }, + "TEST4": &types.Option{ + Default: "${TEST3}-${FOO}-4", + }, + }, + }, + { + Name: "Nested dynamic options skip required", + ProviderOptions: map[string]*types.Option{ + "TEST": { + Default: "test1", + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST2": &types.Option{ + Required: true, + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST3": &types.Option{ + Default: "test3", + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST4": &types.Option{ + Default: "${TEST3}-${FOO}-4", + }, + }), + }, + }), + }, + }), + }, + "FOO": {Command: "echo bar"}, + }, + SkipRequired: true, + ExpectedOptions: map[string]string{ + "TEST": "test1", + "FOO": "bar", + }, + ExpectedDynamicOptions: config.OptionDefinitions{ + "TEST2": &types.Option{ + Required: true, + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST3": &types.Option{ + Default: "test3", + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST4": &types.Option{ + Default: "${TEST3}-${FOO}-4", + }, + }), + }, + }), + }, + }, + }, + { + Name: "Nested dynamic options use option", + ProviderOptions: map[string]*types.Option{ + "TEST": { + Default: "test1", + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST2": &types.Option{ + Required: true, + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST3": &types.Option{ + Default: "test3", + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST4": &types.Option{ + Default: "${TEST2}-${FOO}-4", + }, + }), + }, + }), + }, + }), + }, + "FOO": {Command: "echo bar"}, + }, + SkipRequired: true, + UserValues: map[string]string{ + "TEST2": "test2", + }, + ExpectedOptions: map[string]string{ + "TEST": "test1", + "TEST2": "test2", + "TEST3": "test3", + "TEST4": "test2-bar-4", + "FOO": "bar", + }, + ExpectedDynamicOptions: config.OptionDefinitions{ + "TEST2": &types.Option{ + Required: true, + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST3": &types.Option{ + Default: "test3", + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST4": &types.Option{ + Default: "${TEST2}-${FOO}-4", + }, + }), + }, + }), + }, + "TEST3": &types.Option{ + Default: "test3", + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST4": &types.Option{ + Default: "${TEST2}-${FOO}-4", + }, + }), + }, + "TEST4": &types.Option{ + Default: "${TEST2}-${FOO}-4", + }, + }, + }, + { + Name: "Nested dynamic options use option", + ProviderOptions: map[string]*types.Option{ + "TEST": { + Default: "test1", + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST2": &types.Option{ + Default: "test2", + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST3": &types.Option{ + Default: "test3", + }, + }), + }, + }), + }, + "FOO": {Command: "echo bar"}, + }, + ResolvedValues: map[string]config.OptionValue{ + "TEST5": { + Value: "test5", + }, + }, + ExpectedOptions: map[string]string{ + "TEST": "test1", + "TEST2": "test2", + "TEST3": "test3", + "FOO": "bar", + }, + ExpectedDynamicOptions: config.OptionDefinitions{ + "TEST2": &types.Option{ + Default: "test2", + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST3": &types.Option{ + Default: "test3", + }, + }), + }, + "TEST3": &types.Option{ + Default: "test3", + }, + }, + }, + { + Name: "Dynamic options unused option", + ProviderOptions: map[string]*types.Option{ + "TEST": { + Default: "test1", + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST2": &types.Option{ + Default: "test2", + }, + }), + }, + "FOO": {Command: "echo bar"}, + }, + ResolvedValues: map[string]config.OptionValue{ + "TEST5": { + Value: "test5", + }, + }, + ResolvedDynamicDefinitions: map[string]*types.Option{ + "TEST5": { + Default: "test2", + }, + }, + ExpectedOptions: map[string]string{ + "TEST": "test1", + "TEST2": "test2", + "FOO": "bar", + }, + ExpectedDynamicOptions: config.OptionDefinitions{ + "TEST2": &types.Option{ + Default: "test2", + }, + }, + }, + { + Name: "Dynamic options update default", + ProviderOptions: map[string]*types.Option{ + "TEST": { + Default: "test1", + SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ + "TEST2": &types.Option{ + Default: "test3", + }, + }), + }, + "FOO": {Command: "echo bar"}, + }, + ResolvedValues: map[string]config.OptionValue{ + "TEST": { + Value: "test1", + }, + "TEST2": { + Value: "test2", + }, + }, + ResolvedDynamicDefinitions: map[string]*types.Option{ + "TEST2": { + Default: "test2", + }, + }, + ExpectedOptions: map[string]string{ + "TEST": "test1", + "TEST2": "test3", + "FOO": "bar", + }, + ExpectedDynamicOptions: config.OptionDefinitions{ + "TEST2": &types.Option{ + Default: "test3", + }, + }, + }, +} - for _, tc := range testCases { +func TestResolveOptions(t *testing.T) { + for _, tc := range resolveOptionsTestCases { t.Run(tc.Name, func(t *testing.T) { runResolveTestCase(t, tc) }) diff --git a/pkg/ssh/config_test.go b/pkg/ssh/config_test.go index 03dfe74c0..5330f66a4 100644 --- a/pkg/ssh/config_test.go +++ b/pkg/ssh/config_test.go @@ -15,36 +15,35 @@ func TestSSHConfigSuite(t *testing.T) { suite.Run(t, new(SSHConfigTestSuite)) } -func (s *SSHConfigTestSuite) TestAddHostSection() { - tests := []struct { - name string - config string - execPath string - host string - user string - context string - workspace string - workdir string - command string - gpgagent bool - devsyHome string - provider string - expected string - }{ - { - name: "Basic host addition", - config: "", - execPath: "/path/to/exec", - host: "testhost", - user: "testuser", - context: "testcontext", - workspace: "testworkspace", - workdir: "", - command: "", - gpgagent: false, - devsyHome: "", - provider: "", - expected: `# Devsy Start testhost +var addHostSectionTestCases = []struct { + name string + config string + execPath string + host string + user string + context string + workspace string + workdir string + command string + gpgagent bool + devsyHome string + provider string + expected string +}{ + { + name: "Basic host addition", + config: "", + execPath: "/path/to/exec", + host: "testhost", + user: "testuser", + context: "testcontext", + workspace: "testworkspace", + workdir: "", + command: "", + gpgagent: false, + devsyHome: "", + provider: "", + expected: `# Devsy Start testhost Host testhost ForwardAgent yes LogLevel error @@ -54,21 +53,21 @@ Host testhost ProxyCommand "/path/to/exec" workspace ssh --stdio --context testcontext --user testuser testworkspace User testuser # Devsy End testhost`, - }, - { - name: "AWS provider with ConnectTimeout", - config: "", - execPath: "/path/to/exec", - host: "testhost", - user: "testuser", - context: "testcontext", - workspace: "testworkspace", - workdir: "", - command: "", - gpgagent: false, - devsyHome: "", - provider: "aws", - expected: `# Devsy Start testhost + }, + { + name: "AWS provider with ConnectTimeout", + config: "", + execPath: "/path/to/exec", + host: "testhost", + user: "testuser", + context: "testcontext", + workspace: "testworkspace", + workdir: "", + command: "", + gpgagent: false, + devsyHome: "", + provider: "aws", + expected: `# Devsy Start testhost Host testhost ForwardAgent yes LogLevel error @@ -79,22 +78,22 @@ Host testhost ProxyCommand "/path/to/exec" workspace ssh --stdio --context testcontext --user testuser testworkspace User testuser # Devsy End testhost`, - }, - { - name: "Basic host addition with DEVSY_HOME", - config: "", - execPath: "/path/to/exec", - host: "testhost", - user: "testuser", - context: "testcontext", - workspace: "testworkspace", - workdir: "", - command: "", - gpgagent: false, - devsyHome: "C:\\\\W S\\d", - provider: "", - //nolint:lll // long ProxyCommand expected output - expected: `# Devsy Start testhost + }, + { + name: "Basic host addition with DEVSY_HOME", + config: "", + execPath: "/path/to/exec", + host: "testhost", + user: "testuser", + context: "testcontext", + workspace: "testworkspace", + workdir: "", + command: "", + gpgagent: false, + devsyHome: "C:\\\\W S\\d", + provider: "", + //nolint:lll // long ProxyCommand expected output + expected: `# Devsy Start testhost Host testhost ForwardAgent yes LogLevel error @@ -104,22 +103,22 @@ Host testhost ProxyCommand "/path/to/exec" workspace ssh --stdio --context testcontext --user testuser testworkspace --home "C:\\W S\d" User testuser # Devsy End testhost`, - }, - { - name: "Host addition with workdir", - config: "", - execPath: "/path/to/exec", - host: "testhost", - user: "testuser", - context: "testcontext", - workspace: "testworkspace", - workdir: "/path/to/workdir", - command: "", - gpgagent: false, - devsyHome: "", - provider: "", - //nolint:lll // long ProxyCommand expected output - expected: `# Devsy Start testhost + }, + { + name: "Host addition with workdir", + config: "", + execPath: "/path/to/exec", + host: "testhost", + user: "testuser", + context: "testcontext", + workspace: "testworkspace", + workdir: "/path/to/workdir", + command: "", + gpgagent: false, + devsyHome: "", + provider: "", + //nolint:lll // long ProxyCommand expected output + expected: `# Devsy Start testhost Host testhost ForwardAgent yes LogLevel error @@ -129,22 +128,22 @@ Host testhost ProxyCommand "/path/to/exec" workspace ssh --stdio --context testcontext --user testuser testworkspace --workdir "/path/to/workdir" User testuser # Devsy End testhost`, - }, - { - name: "Host addition with gpg agent", - config: "", - execPath: "/path/to/exec", - host: "testhost", - user: "testuser", - context: "testcontext", - workspace: "testworkspace", - workdir: "", - command: "", - gpgagent: true, - devsyHome: "", - provider: "", - //nolint:lll // long ProxyCommand expected output - expected: `# Devsy Start testhost + }, + { + name: "Host addition with gpg agent", + config: "", + execPath: "/path/to/exec", + host: "testhost", + user: "testuser", + context: "testcontext", + workspace: "testworkspace", + workdir: "", + command: "", + gpgagent: true, + devsyHome: "", + provider: "", + //nolint:lll // long ProxyCommand expected output + expected: `# Devsy Start testhost Host testhost ForwardAgent yes LogLevel error @@ -154,21 +153,21 @@ Host testhost ProxyCommand "/path/to/exec" workspace ssh --stdio --context testcontext --user testuser testworkspace --ssh-gpg-forwarding User testuser # Devsy End testhost`, - }, - { - name: "Host addition with custom command", - config: "", - execPath: "/path/to/exec", - host: "testhost", - user: "testuser", - context: "testcontext", - workspace: "testworkspace", - workdir: "", - command: "ssh -W %h:%p bastion", - gpgagent: false, - devsyHome: "", - provider: "", - expected: `# Devsy Start testhost + }, + { + name: "Host addition with custom command", + config: "", + execPath: "/path/to/exec", + host: "testhost", + user: "testuser", + context: "testcontext", + workspace: "testworkspace", + workdir: "", + command: "ssh -W %h:%p bastion", + gpgagent: false, + devsyHome: "", + provider: "", + expected: `# Devsy Start testhost Host testhost ForwardAgent yes LogLevel error @@ -178,22 +177,22 @@ Host testhost ProxyCommand "ssh -W %h:%p bastion" User testuser # Devsy End testhost`, - }, - { - name: "Host addition to existing config", - config: `Host existinghost + }, + { + name: "Host addition to existing config", + config: `Host existinghost User existinguser`, - execPath: "/path/to/exec", - host: "testhost", - user: "testuser", - context: "testcontext", - workspace: "testworkspace", - workdir: "", - command: "", - gpgagent: false, - devsyHome: "", - provider: "", - expected: `# Devsy Start testhost + execPath: "/path/to/exec", + host: "testhost", + user: "testuser", + context: "testcontext", + workspace: "testworkspace", + workdir: "", + command: "", + gpgagent: false, + devsyHome: "", + provider: "", + expected: `# Devsy Start testhost Host testhost ForwardAgent yes LogLevel error @@ -205,10 +204,10 @@ Host testhost # Devsy End testhost Host existinghost User existinguser`, - }, - { - name: "Host addition to existing config with Devsy host", - config: `# Devsy Start existingtesthost + }, + { + name: "Host addition to existing config with Devsy host", + config: `# Devsy Start existingtesthost Host existingtesthost ForwardAgent yes LogLevel error @@ -221,17 +220,17 @@ Host existingtesthost Host existinghost User existinguser`, - execPath: "/path/to/exec", - host: "testhost", - user: "testuser", - context: "testcontext", - workspace: "testworkspace", - workdir: "", - command: "", - gpgagent: false, - devsyHome: "", - provider: "", - expected: `# Devsy Start testhost + execPath: "/path/to/exec", + host: "testhost", + user: "testuser", + context: "testcontext", + workspace: "testworkspace", + workdir: "", + command: "", + gpgagent: false, + devsyHome: "", + provider: "", + expected: `# Devsy Start testhost Host testhost ForwardAgent yes LogLevel error @@ -254,27 +253,27 @@ Host existingtesthost Host existinghost User existinguser`, - }, - { - name: "Host addition after top level includes", - config: `Include ~/config1 + }, + { + name: "Host addition after top level includes", + config: `Include ~/config1 Include ~/config2 Include ~/config3`, - execPath: "/path/to/exec", - host: "testhost", - user: "testuser", - context: "testcontext", - workspace: "testworkspace", - workdir: "", - command: "", - gpgagent: false, - devsyHome: "", - provider: "", - expected: `Include ~/config1 + execPath: "/path/to/exec", + host: "testhost", + user: "testuser", + context: "testcontext", + workspace: "testworkspace", + workdir: "", + command: "", + gpgagent: false, + devsyHome: "", + provider: "", + expected: `Include ~/config1 Include ~/config2 @@ -291,10 +290,11 @@ Host testhost ProxyCommand "/path/to/exec" workspace ssh --stdio --context testcontext --user testuser testworkspace User testuser # Devsy End testhost`, - }, - } + }, +} - for _, tt := range tests { +func (s *SSHConfigTestSuite) TestAddHostSection() { + for _, tt := range addHostSectionTestCases { s.Run(tt.name, func() { result, err := addHostSection(tt.config, tt.execPath, addHostParams{ path: "", diff --git a/pkg/types/types_test.go b/pkg/types/types_test.go index dcdbcbbcf..494653f41 100644 --- a/pkg/types/types_test.go +++ b/pkg/types/types_test.go @@ -8,20 +8,22 @@ import ( "gotest.tools/assert" ) -func TestLifecycleHookUnmarshalJSON(t *testing.T) { - type input struct { - Input types.LifecycleHook `json:"input,omitempty"` - } +type lifecycleHookUnmarshalInput struct { + Input types.LifecycleHook `json:"input,omitempty"` +} - testCases := []struct { - Name string - Input string - Expect input - }{ +type lifecycleHookUnmarshalCase struct { + Name string + Input string + Expect lifecycleHookUnmarshalInput +} + +func lifecycleHookUnmarshalTestCases() []lifecycleHookUnmarshalCase { + return []lifecycleHookUnmarshalCase{ { Name: "string", Input: `{"input": "some-string"}`, - Expect: input{ + Expect: lifecycleHookUnmarshalInput{ Input: types.LifecycleHook{ "": []string{"some-string"}, }, @@ -30,7 +32,7 @@ func TestLifecycleHookUnmarshalJSON(t *testing.T) { { Name: "array of strings", Input: `{"input": ["string1", "string2"]}`, - Expect: input{ + Expect: lifecycleHookUnmarshalInput{ Input: types.LifecycleHook{ "": []string{ "string1", @@ -42,7 +44,7 @@ func TestLifecycleHookUnmarshalJSON(t *testing.T) { { Name: "object of strings", Input: `{"input": {"key1": "value1", "key2": "value2"}}`, - Expect: input{ + Expect: lifecycleHookUnmarshalInput{ Input: types.LifecycleHook{ "key1": []string{ "value1", @@ -56,7 +58,7 @@ func TestLifecycleHookUnmarshalJSON(t *testing.T) { { Name: "object of array of strings", Input: `{"input": {"key1": ["value1","value2"], "key2": ["value3","value4"]}}`, - Expect: input{ + Expect: lifecycleHookUnmarshalInput{ Input: types.LifecycleHook{ "key1": []string{ "value1", @@ -70,10 +72,12 @@ func TestLifecycleHookUnmarshalJSON(t *testing.T) { }, }, } +} - for _, testCase := range testCases { +func TestLifecycleHookUnmarshalJSON(t *testing.T) { + for _, testCase := range lifecycleHookUnmarshalTestCases() { t.Run(testCase.Name, func(t *testing.T) { - var data input + var data lifecycleHookUnmarshalInput err := json.Unmarshal([]byte(testCase.Input), &data) assert.NilError(t, err, testCase.Name) diff --git a/pkg/workspace/id_test.go b/pkg/workspace/id_test.go index ff951e482..3fea7f6e7 100644 --- a/pkg/workspace/id_test.go +++ b/pkg/workspace/id_test.go @@ -9,8 +9,12 @@ import ( // Note that these tests document the status quo and not the ideal state. // I've created follow up tickets to adjust the ToID function and update the tests but because // this is a potentially breaking change we'll have to wait for the next major version. -func TestToID(t *testing.T) { - tests := []struct { +func toIDTestCases() []struct { + name string + input string + want string +} { + return []struct { name string input string want string @@ -66,8 +70,10 @@ func TestToID(t *testing.T) { want: "devsyreallylongreponamethatexceeds48charactersto", }, } +} - for _, tt := range tests { +func TestToID(t *testing.T) { + for _, tt := range toIDTestCases() { t.Run(tt.name, func(t *testing.T) { got := ToID(tt.input) if got != tt.want { From a7dba0539aa5224d661f111f48b90fd6a60a4b09 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 6 Aug 2026 15:40:44 +0000 Subject: [PATCH 4/5] fix(lint): resolve cross-linter regressions from funlen extraction The funlen extraction in the prior commits introduced real new violations of other linters, all caught by the CI-equivalent check (golangci-lint run --new-from-patch=$(git diff $(git merge-base HEAD origin/main)) --new=false ./...), not just --enable-only=funlen: - forbidigo: removed a stray fmt.Println(testCase.Name) debug print in pkg/options/options_test.go, pre-existing on main but only surfaced as 'new' because relocation shifted its diff hunk; t.Run's subtest naming already covers this, no test coverage lost. Removed the now-unused fmt import too. - funcorder: moved the register*Flags helper methods added by the Group A funlen commit to after Run/Close in cmd/internal/agentcontainer/setup.go, cmd/internal/runusercommands.go, cmd/workspace/ssh.go, and swapped logOutputStreams.stdout/Close order in pkg/client/clientimplementation/daemonclient/up.go -- exported methods must precede unexported ones on the same receiver. - revive (argument-limit / function-result-limit): bundled the new helpers' parameters/results into Params/Result structs, matching the existing convention (loginParams, waitForWorkspacePhaseParams, etc.) established for the dupl-findings PR -- cmd/workspace/logs.go (injectLogsAgent), cmd/workspace/ssh.go (runInteractiveTunnelSession), pkg/client/clientimplementation/daemonclient/form.go (selectProjectClusterTemplate, buildNewInstance), pkg/devcontainer/ compose_build.go (resolveComposeBuildTarget), pkg/options/resolve.go (applyResolvedProviderOptions), pkg/platform/form/form.go (runCreateSelectionForm, buildCreatedInstance), pkg/platform/kubeconfig.go (newVClusterKubeConfigRequest, directVirtualClusterKubeConfig). While fixing this in pkg/client/clientimplementation/daemonclient/form.go I caught and fixed a self-inflicted bug: an earlier truncated read had caused buildNewInstance to silently drop the Target and Parameters fields from the constructed DevsyWorkspaceInstance -- restored, verified against origin/main's field list. - gosec / lll: the two flagged spots (TLS InsecureSkipVerify and one long URL format string in cmd/internal/agentcontainer/setup.go) are unchanged, pre-existing content that only look 'new' because extraction shifted their indentation. Suppressed with //nolint citing this, rather than fixing gosec/lll findings that belong to their own future backlog PRs. - goconst: pkg/ssh/config_test.go's relocated table now duplicates 5 string literals that already have named constants elsewhere in the package (testExecPath, testHostBasic, testUser, testContextAlt, testWorkspaceAlt) -- swapped to reuse them. pkg/options/options_test.go and pkg/options/resolve_test.go's relocated tables hoisted every genuinely repeated literal into real named constants (not just to silence the linter): testOptTest, testValTest, testOptCommand, testRefChain34, etc. -- 26 total across both files. Re-verified with the exact CI check (not just --enable-only=funlen): 0 issues. go build, go vet (clean aside from the same pre-existing unrelated pkg/pty/ptytest finding from prior rounds), and the full test suite for every touched package all pass -- spot-checked TestResolveOptions (22 subtests) and TestInheritFromEnvironment (4 subtests) individually, not just package exit codes. --- cmd/internal/agentcontainer/setup.go | 71 +-- cmd/internal/runusercommands.go | 78 +-- cmd/workspace/logs.go | 40 +- cmd/workspace/ssh.go | 127 ++--- .../clientimplementation/daemonclient/form.go | 94 ++-- .../clientimplementation/daemonclient/up.go | 8 +- pkg/devcontainer/compose_build.go | 40 +- pkg/options/options_test.go | 42 +- pkg/options/resolve.go | 39 +- pkg/options/resolve_test.go | 495 ++++++++++-------- pkg/platform/form/form.go | 79 +-- pkg/platform/kubeconfig.go | 73 +-- pkg/ssh/config_test.go | 90 ++-- 13 files changed, 684 insertions(+), 592 deletions(-) diff --git a/cmd/internal/agentcontainer/setup.go b/cmd/internal/agentcontainer/setup.go index 75c59db61..fae899201 100644 --- a/cmd/internal/agentcontainer/setup.go +++ b/cmd/internal/agentcontainer/setup.go @@ -81,6 +81,40 @@ func NewSetupContainerCmd(globalFlags *flags.GlobalFlags) *cobra.Command { return setupContainerCmd } +type setupContext struct { + ctx context.Context + workspaceInfo *provider2.ContainerWorkspaceInfo + setupInfo *config.Result + tunnelClient tunnel.TunnelClient + secretsEnv []string +} + +// Run runs the command logic. +func (cmd *SetupContainerCmd) Run(ctx context.Context) error { + tunnelClient, err := cmd.initializeTunnelClient(ctx) + if err != nil { + return err + } + + workspaceInfo, setupInfo, err := cmd.parseWorkspaceAndSetupInfo() + if err != nil { + return err + } + + sctx := &setupContext{ + ctx: ctx, + workspaceInfo: workspaceInfo, + setupInfo: setupInfo, + tunnelClient: tunnelClient, + } + + if err := cmd.prepareWorkspace(sctx); err != nil { + return err + } + + return cmd.finalizeSetup(sctx) +} + func (cmd *SetupContainerCmd) registerFlags(setupContainerCmd *cobra.Command) { cmd.registerBehaviorFlags(setupContainerCmd) cmd.registerWorkspaceInfoFlags(setupContainerCmd) @@ -150,40 +184,6 @@ func (cmd *SetupContainerCmd) registerDotfilesFlags(setupContainerCmd *cobra.Com ) } -type setupContext struct { - ctx context.Context - workspaceInfo *provider2.ContainerWorkspaceInfo - setupInfo *config.Result - tunnelClient tunnel.TunnelClient - secretsEnv []string -} - -// Run runs the command logic. -func (cmd *SetupContainerCmd) Run(ctx context.Context) error { - tunnelClient, err := cmd.initializeTunnelClient(ctx) - if err != nil { - return err - } - - workspaceInfo, setupInfo, err := cmd.parseWorkspaceAndSetupInfo() - if err != nil { - return err - } - - sctx := &setupContext{ - ctx: ctx, - workspaceInfo: workspaceInfo, - setupInfo: setupInfo, - tunnelClient: tunnelClient, - } - - if err := cmd.prepareWorkspace(sctx); err != nil { - return err - } - - return cmd.finalizeSetup(sctx) -} - func (cmd *SetupContainerCmd) prepareWorkspace(sctx *setupContext) error { if err := cmd.syncMounts(sctx); err != nil { return err @@ -961,6 +961,7 @@ func streamMountFromPlatform( httpClient := &http.Client{ Transport: &http.Transport{ TLSClientConfig: &tls.Config{ + //nolint:gosec // pre-existing, relocated by funlen extraction; out of scope for this PR InsecureSkipVerify: true, }, }, @@ -1000,7 +1001,7 @@ func buildPlatformDownloadRequest( m *config.Mount, ) (*http.Request, error) { downloadURL := fmt.Sprintf( - "https://%s/kubernetes/management/apis/management.devsy.sh/v1/namespaces/%s/devsyworkspaceinstances/%s/download?path=%s", + "https://%s/kubernetes/management/apis/management.devsy.sh/v1/namespaces/%s/devsyworkspaceinstances/%s/download?path=%s", //nolint:lll // pre-existing, relocated by funlen extraction; out of scope for this PR ts.RemoveProtocol(workspaceInfo.CLIOptions.Platform.PlatformHost), workspaceInfo.CLIOptions.Platform.InstanceNamespace, workspaceInfo.CLIOptions.Platform.InstanceName, diff --git a/cmd/internal/runusercommands.go b/cmd/internal/runusercommands.go index 516b0e9ab..5113a206e 100644 --- a/cmd/internal/runusercommands.go +++ b/cmd/internal/runusercommands.go @@ -61,6 +61,45 @@ func NewRunUserCommandsCmd(f *flags.GlobalFlags) *cobra.Command { return runCmd } +// NewRunUserCommandsCmdAlias creates the hidden camelCase alias for devcontainer CLI compat. +func NewRunUserCommandsCmdAlias(f *flags.GlobalFlags) *cobra.Command { + primary := NewRunUserCommandsCmd(f) + primary.Use = "runUserCommands" + primary.Hidden = true + return primary +} + +const updateContentCommand = "updateContentCommand" + +// Run executes the run-user-commands logic. +func (cmd *RunUserCommandsCmd) Run(ctx context.Context) error { + if err := cmd.validate(); err != nil { + return err + } + + if cmd.ContainerID != "" { + return cmd.runWithContainerID(ctx) + } + + params, result, err := cmd.resolveContainer(ctx) + if err != nil { + return err + } + + if err := cmd.runLifecycleHooks(params, result); err != nil { + return err + } + + user := devcconfig.GetRemoteUser(result) + log.Infof("lifecycle commands completed for container %s", params.ContainerID) + _ = devcconfig.WriteResultJSON(os.Stderr, devcconfig.ResultEnvelope{ + ContainerID: params.ContainerID, + RemoteUser: user, + RemoteWorkspaceFolder: params.Workdir, + }) + return nil +} + func (cmd *RunUserCommandsCmd) registerFlags(runCmd *cobra.Command) { cmd.registerTargetFlags(runCmd) cmd.registerConfigFlags(runCmd) @@ -171,45 +210,6 @@ func (cmd *RunUserCommandsCmd) registerLifecycleFlags(runCmd *cobra.Command) { ) } -// NewRunUserCommandsCmdAlias creates the hidden camelCase alias for devcontainer CLI compat. -func NewRunUserCommandsCmdAlias(f *flags.GlobalFlags) *cobra.Command { - primary := NewRunUserCommandsCmd(f) - primary.Use = "runUserCommands" - primary.Hidden = true - return primary -} - -const updateContentCommand = "updateContentCommand" - -// Run executes the run-user-commands logic. -func (cmd *RunUserCommandsCmd) Run(ctx context.Context) error { - if err := cmd.validate(); err != nil { - return err - } - - if cmd.ContainerID != "" { - return cmd.runWithContainerID(ctx) - } - - params, result, err := cmd.resolveContainer(ctx) - if err != nil { - return err - } - - if err := cmd.runLifecycleHooks(params, result); err != nil { - return err - } - - user := devcconfig.GetRemoteUser(result) - log.Infof("lifecycle commands completed for container %s", params.ContainerID) - _ = devcconfig.WriteResultJSON(os.Stderr, devcconfig.ResultEnvelope{ - ContainerID: params.ContainerID, - RemoteUser: user, - RemoteWorkspaceFolder: params.Workdir, - }) - return nil -} - func (cmd *RunUserCommandsCmd) validate() error { if cmd.ContainerID != "" && cmd.WorkspaceFolder == "" && cmd.Config == "" { return fmt.Errorf( diff --git a/cmd/workspace/logs.go b/cmd/workspace/logs.go index bdb9c3286..3121d124d 100644 --- a/cmd/workspace/logs.go +++ b/cmd/workspace/logs.go @@ -79,7 +79,13 @@ func (cmd *LogsCmd) Run(ctx context.Context, args []string) error { return pb.RunPair(ctx, func(ctx context.Context, stdin, stdout *os.File) error { - return injectLogsAgent(ctx, client, sshServerCmd, timeout, stdin, stdout) + return injectLogsAgent(ctx, injectLogsAgentParams{ + client: client, + sshServerCmd: sshServerCmd, + timeout: timeout, + stdin: stdin, + stdout: stdout, + }) }, func(ctx context.Context, stdout, stdin *os.File) error { return runLogsSession(stdout, stdin, client) @@ -111,37 +117,39 @@ func (cmd *LogsCmd) getWorkspaceClient( // injectLogsAgent injects the devsy agent binary over stdin/stdout and runs the // remote ssh-server that runLogsSession then connects to. -func injectLogsAgent( - ctx context.Context, - client clientpkg.WorkspaceClient, - sshServerCmd string, - timeout time.Duration, - stdin, stdout *os.File, -) error { +func injectLogsAgent(ctx context.Context, params injectLogsAgentParams) error { stderr := log.Writer(log.LevelDebug) defer func() { _ = stderr.Close() }() return agent.InjectAgent(&agent.InjectOptions{ Ctx: ctx, Exec: func(ctx context.Context, command string, stdinR io.Reader, stdoutW io.Writer, stderrW io.Writer) error { - return client.Command(ctx, clientpkg.CommandOptions{ + return params.client.Command(ctx, clientpkg.CommandOptions{ Command: command, Stdin: stdinR, Stdout: stdoutW, Stderr: stderrW, }) }, - IsLocal: client.AgentLocal(), - RemoteAgentPath: client.AgentPath(), - DownloadURL: client.AgentURL(), - Command: sshServerCmd, - Stdin: stdin, - Stdout: stdout, + IsLocal: params.client.AgentLocal(), + RemoteAgentPath: params.client.AgentPath(), + DownloadURL: params.client.AgentURL(), + Command: params.sshServerCmd, + Stdin: params.stdin, + Stdout: params.stdout, Stderr: stderr, - Timeout: timeout, + Timeout: params.timeout, }) } +type injectLogsAgentParams struct { + client clientpkg.WorkspaceClient + sshServerCmd string + timeout time.Duration + stdin *os.File + stdout *os.File +} + func runLogsSession(stdout, stdin *os.File, client clientpkg.WorkspaceClient) error { sshClient, err := ssh.StdioClientWithUser(stdout, stdin, "", false) if err != nil { diff --git a/cmd/workspace/ssh.go b/cmd/workspace/ssh.go index 745ddc6de..9e6c03bb6 100644 --- a/cmd/workspace/ssh.go +++ b/cmd/workspace/ssh.go @@ -89,6 +89,48 @@ func NewSSHCmd(f *flags.GlobalFlags) *cobra.Command { return sshCmd } +// Run runs the command logic. +func (cmd *SSHCmd) Run( + ctx context.Context, + devsyConfig *config.Config, + client client2.BaseWorkspaceClient, +) error { + cmd.addPrivateKeysToAgentIfEnabled(ctx, devsyConfig) + + // get user + if cmd.User == "" { + var err error + cmd.User, err = devssh.GetUser( + client.WorkspaceConfig().ID, + client.WorkspaceConfig().SSHConfigPath, + client.WorkspaceConfig().SSHConfigIncludePath, + ) + if err != nil { + return err + } + } + + // set default context if needed + if cmd.Context == "" { + cmd.Context = devsyConfig.DefaultContext + } + + workspaceClient, ok := client.(client2.WorkspaceClient) + if ok { + return cmd.jumpContainer(ctx, devsyConfig, workspaceClient) + } + proxyClient, ok := client.(client2.ProxyClient) + if ok { + return cmd.startProxyTunnel(ctx, devsyConfig, proxyClient) + } + daemonClient, ok := client.(client2.DaemonClient) + if ok { + return cmd.jumpContainerTailscale(ctx, devsyConfig, daemonClient) + } + + return nil +} + func (cmd *SSHCmd) registerFlags(sshCmd *cobra.Command) { cmd.registerPortForwardingFlags(sshCmd) cmd.registerEnvFlags(sshCmd) @@ -192,48 +234,6 @@ func (cmd *SSHCmd) registerTerminalFlags(sshCmd *cobra.Command) { ) } -// Run runs the command logic. -func (cmd *SSHCmd) Run( - ctx context.Context, - devsyConfig *config.Config, - client client2.BaseWorkspaceClient, -) error { - cmd.addPrivateKeysToAgentIfEnabled(ctx, devsyConfig) - - // get user - if cmd.User == "" { - var err error - cmd.User, err = devssh.GetUser( - client.WorkspaceConfig().ID, - client.WorkspaceConfig().SSHConfigPath, - client.WorkspaceConfig().SSHConfigIncludePath, - ) - if err != nil { - return err - } - } - - // set default context if needed - if cmd.Context == "" { - cmd.Context = devsyConfig.DefaultContext - } - - workspaceClient, ok := client.(client2.WorkspaceClient) - if ok { - return cmd.jumpContainer(ctx, devsyConfig, workspaceClient) - } - proxyClient, ok := client.(client2.ProxyClient) - if ok { - return cmd.startProxyTunnel(ctx, devsyConfig, proxyClient) - } - daemonClient, ok := client.(client2.DaemonClient) - if ok { - return cmd.jumpContainerTailscale(ctx, devsyConfig, daemonClient) - } - - return nil -} - func (cmd *SSHCmd) addPrivateKeysToAgentIfEnabled(ctx context.Context, devsyConfig *config.Config) { if devsyConfig.ContextOption(config.ContextOptionSSHAgentForwarding) != config.BoolTrue || devsyConfig.ContextOption(config.ContextOptionSSHAddPrivateKeys) != config.BoolTrue { @@ -457,14 +457,13 @@ func (cmd *SSHCmd) startTunnel( }) } - return cmd.runInteractiveTunnelSession( - ctx, - devsyConfig, - containerClient, - command, - envVars, - writer, - ) + return cmd.runInteractiveTunnelSession(ctx, runInteractiveTunnelSessionParams{ + devsyConfig: devsyConfig, + containerClient: containerClient, + command: command, + envVars: envVars, + writer: writer, + }) } // setupTunnelWriter wires up the JSON log pipe and GPG agent tunnel shared by @@ -490,37 +489,43 @@ func (cmd *SSHCmd) setupTunnelWriter( } } +type runInteractiveTunnelSessionParams struct { + devsyConfig *config.Config + containerClient *ssh.Client + command string + envVars map[string]string + writer io.Writer +} + func (cmd *SSHCmd) runInteractiveTunnelSession( ctx context.Context, - devsyConfig *config.Config, - containerClient *ssh.Client, - command string, - envVars map[string]string, - writer io.Writer, + params runInteractiveTunnelSessionParams, ) error { return machine.StartSSHSession(ctx, machine.StartSSHSessionOptions{ User: cmd.User, Command: cmd.Command, AgentForwarding: cmd.AgentForwarding && - devsyConfig.ContextOption(config.ContextOptionSSHAgentForwarding) == config.BoolTrue, + params.devsyConfig.ContextOption( + config.ContextOptionSSHAgentForwarding, + ) == config.BoolTrue, SessionOptions: machine.SSHSessionOptions{ TermMode: cmd.TermMode, InstallTerminfo: cmd.InstallTerminfo, }, Exec: func(ctx context.Context, stdin io.Reader, stdout io.Writer, stderr io.Writer) error { if cmd.SSHKeepAliveInterval != DisableSSHKeepAlive { - go startSSHKeepAlive(ctx, containerClient, cmd.SSHKeepAliveInterval) + go startSSHKeepAlive(ctx, params.containerClient, cmd.SSHKeepAliveInterval) } return devssh.Run(ctx, devssh.RunOptions{ - Client: containerClient, - Command: command, + Client: params.containerClient, + Command: params.command, Stdin: stdin, Stdout: stdout, Stderr: stderr, - EnvVars: envVars, + EnvVars: params.envVars, }) }, - Stderr: writer, + Stderr: params.writer, }) } diff --git a/pkg/client/clientimplementation/daemonclient/form.go b/pkg/client/clientimplementation/daemonclient/form.go index a0e5a7aed..af823a917 100644 --- a/pkg/client/clientimplementation/daemonclient/form.go +++ b/pkg/client/clientimplementation/daemonclient/form.go @@ -29,43 +29,45 @@ func createInstanceInteractive( formCtx, cancelForm := context.WithCancel(ctx) defer cancelForm() - selectedProject, selectedCluster, selectedTemplate, selectedTemplateVersion, err := selectProjectClusterTemplate( - ctx, - formCtx, - baseClient, - cancelForm, - ) + selection, err := selectProjectClusterTemplate(ctx, formCtx, baseClient, cancelForm) if err != nil { return nil, err } renderedParameters, err := resolveNewInstanceParameters( formCtx, - selectedTemplate, - selectedTemplateVersion, + selection.template, + selection.templateVersion, ) if err != nil { return nil, err } - return buildNewInstance( - id, - uid, - source, - picture, - selectedProject, - selectedCluster, - selectedTemplate, - selectedTemplateVersion, - renderedParameters, - ), nil + return buildNewInstance(buildNewInstanceParams{ + id: id, + uid: uid, + source: source, + picture: picture, + selectedProject: selection.project, + selectedCluster: selection.cluster, + selectedTemplate: selection.template, + selectedTemplateVersion: selection.templateVersion, + renderedParameters: renderedParameters, + }), nil +} + +type projectClusterTemplateSelection struct { + project *managementv1.Project + cluster *managementv1.Cluster + template *managementv1.DevsyWorkspaceTemplate + templateVersion string } func selectProjectClusterTemplate( ctx, formCtx context.Context, baseClient platformclient.Client, cancelForm CancelFunc, -) (*managementv1.Project, *managementv1.Cluster, *managementv1.DevsyWorkspaceTemplate, string, error) { +) (projectClusterTemplateSelection, error) { var selectedCluster *managementv1.Cluster var selectedProject *managementv1.Project var selectedTemplate *managementv1.DevsyWorkspaceTemplate @@ -73,7 +75,7 @@ func selectProjectClusterTemplate( options, err := projectOptions(ctx, baseClient) if err != nil { - return nil, nil, nil, "", err + return projectClusterTemplateSelection{}, err } err = huh.NewForm( @@ -105,10 +107,15 @@ func selectProjectClusterTemplate( ), ).RunWithContext(formCtx) if err != nil { - return nil, nil, nil, "", err + return projectClusterTemplateSelection{}, err } - return selectedProject, selectedCluster, selectedTemplate, selectedTemplateVersion, nil + return projectClusterTemplateSelection{ + project: selectedProject, + cluster: selectedCluster, + template: selectedTemplate, + templateVersion: selectedTemplateVersion, + }, nil } func resolveNewInstanceParameters( @@ -138,40 +145,43 @@ func resolveNewInstanceParameters( return renderParameters(fieldParameters) } -func buildNewInstance( - id, uid, source, picture string, - selectedProject *managementv1.Project, - selectedCluster *managementv1.Cluster, - selectedTemplate *managementv1.DevsyWorkspaceTemplate, - selectedTemplateVersion, renderedParameters string, -) *managementv1.DevsyWorkspaceInstance { +type buildNewInstanceParams struct { + id, uid, source, picture string + selectedProject *managementv1.Project + selectedCluster *managementv1.Cluster + selectedTemplate *managementv1.DevsyWorkspaceTemplate + selectedTemplateVersion string + renderedParameters string +} + +func buildNewInstance(p buildNewInstanceParams) *managementv1.DevsyWorkspaceInstance { return &managementv1.DevsyWorkspaceInstance{ ObjectMeta: metav1.ObjectMeta{ - GenerateName: encoding.SafeConcatNameMax([]string{id}, 53) + "-", - Namespace: project.ProjectNamespace(selectedProject.GetName()), + GenerateName: encoding.SafeConcatNameMax([]string{p.id}, 53) + "-", + Namespace: project.ProjectNamespace(p.selectedProject.GetName()), Labels: map[string]string{ - storagev1.DevsyWorkspaceIDLabel: id, - storagev1.DevsyWorkspaceUIDLabel: uid, - config.K8sProjectLabel: selectedProject.GetName(), + storagev1.DevsyWorkspaceIDLabel: p.id, + storagev1.DevsyWorkspaceUIDLabel: p.uid, + config.K8sProjectLabel: p.selectedProject.GetName(), }, Annotations: map[string]string{ - storagev1.DevsyWorkspacePictureAnnotation: picture, - storagev1.DevsyWorkspaceSourceAnnotation: source, + storagev1.DevsyWorkspacePictureAnnotation: p.picture, + storagev1.DevsyWorkspaceSourceAnnotation: p.source, }, }, Spec: managementv1.DevsyWorkspaceInstanceSpec{ DevsyWorkspaceInstanceSpec: storagev1.DevsyWorkspaceInstanceSpec{ - DisplayName: id, + DisplayName: p.id, TemplateRef: &storagev1.TemplateRef{ - Name: selectedTemplate.GetName(), - Version: selectedTemplateVersion, + Name: p.selectedTemplate.GetName(), + Version: p.selectedTemplateVersion, }, Target: storagev1.WorkspaceTarget{ Cluster: &storagev1.WorkspaceTargetName{ - Name: selectedCluster.GetName(), + Name: p.selectedCluster.GetName(), }, }, - Parameters: renderedParameters, + Parameters: p.renderedParameters, }, }, } diff --git a/pkg/client/clientimplementation/daemonclient/up.go b/pkg/client/clientimplementation/daemonclient/up.go index fdbfdac01..e3cea9df7 100644 --- a/pkg/client/clientimplementation/daemonclient/up.go +++ b/pkg/client/clientimplementation/daemonclient/up.go @@ -444,10 +444,6 @@ func newLogOutputStreams(reporter status.Reporter) *logOutputStreams { } } -func (s *logOutputStreams) stdout() io.Writer { - return s.statusWriter -} - func (s *logOutputStreams) Close() { _ = s.statusWriter.Close() _ = s.stdoutStreamer.Close() @@ -456,6 +452,10 @@ func (s *logOutputStreams) Close() { <-s.stderrDone } +func (s *logOutputStreams) stdout() io.Writer { + return s.statusWriter +} + // streamLogMessages reads NDJSON-encoded Message lines from scanner and // writes their payloads to stdout/stderr until an ExitCode message, a write // error, or EOF is reached. done reports whether the caller should return diff --git a/pkg/devcontainer/compose_build.go b/pkg/devcontainer/compose_build.go index ce1ff659f..79af1ddae 100644 --- a/pkg/devcontainer/compose_build.go +++ b/pkg/devcontainer/compose_build.go @@ -126,17 +126,14 @@ func (r *runner) buildAndExtendDockerCompose( return composeExtendResult{}, err } - buildImageName, dockerComposeFilePath, cleanup, err := r.resolveComposeBuildTarget( - prepared, - params, - ) - defer cleanup() + target, err := r.resolveComposeBuildTarget(prepared, params) + defer target.cleanup() if err != nil { - return composeExtendResult{buildImageName: buildImageName}, err + return composeExtendResult{buildImageName: target.buildImageName}, err } - if err := r.runComposeExtendedBuild(ctx, params, dockerComposeFilePath); err != nil { - return composeExtendResult{buildImageName: buildImageName}, err + if err := r.runComposeExtendedBuild(ctx, params, target.dockerComposeFilePath); err != nil { + return composeExtendResult{buildImageName: target.buildImageName}, err } imageMetadata, err := metadata.GetDevContainerMetadata( @@ -146,42 +143,51 @@ func (r *runner) buildAndExtendDockerCompose( prepared.extendImageBuildInfo.Features, ) if err != nil { - return composeExtendResult{buildImageName: buildImageName}, err + return composeExtendResult{buildImageName: target.buildImageName}, err } return composeExtendResult{ - buildImageName: buildImageName, - composeBuildFilePath: dockerComposeFilePath, + buildImageName: target.buildImageName, + composeBuildFilePath: target.dockerComposeFilePath, imageMetadata: imageMetadata, metadataLabel: prepared.extendImageBuildInfo.MetadataLabel, }, nil } +// composeBuildTarget holds the resolved image name, override compose file +// path (if any), and cleanup for the feature override compose file. +type composeBuildTarget struct { + buildImageName string + dockerComposeFilePath string + cleanup func() +} + // resolveComposeBuildTarget determines the image name to build and, when the // build has features, writes the feature override compose file to build // against. The returned cleanup func is always non-nil and safe to defer. func (r *runner) resolveComposeBuildTarget( prepared preparedComposeBuild, params *buildAndExtendParams, -) (buildImageName string, dockerComposeFilePath string, cleanup func(), err error) { - cleanup = func() {} +) (composeBuildTarget, error) { + target := composeBuildTarget{cleanup: func() {}} - buildImageName, err = composeBuildImageName( + buildImageName, err := composeBuildImageName( params.composeHelper, params.project.Name, params.composeService, hasFeatureBuildInfo(prepared.extendImageBuildInfo), ) + target.buildImageName = buildImageName if err != nil { - return buildImageName, "", cleanup, err + return target, err } - dockerComposeFilePath, cleanup, err = r.composeFeatureOverride( + target.dockerComposeFilePath, target.cleanup, err = r.composeFeatureOverride( prepared, params.composeService, buildImageName, ) - return buildImageName, dockerComposeFilePath, cleanup, err + return target, err } // runComposeExtendedBuild assembles the compose build arguments and runs diff --git a/pkg/options/options_test.go b/pkg/options/options_test.go index e9c857676..aaa91ebb9 100644 --- a/pkg/options/options_test.go +++ b/pkg/options/options_test.go @@ -1,7 +1,6 @@ package options import ( - "fmt" "os" "testing" @@ -20,33 +19,40 @@ type assignmentTestCase struct { ExpectedAssignments []string } +const ( + testEnvHostName = "HOST" + testEnvHostAssign = "HOST=box" + testEnvSSHPrefix = "DEVSY_PROVIDER_SSH_" + testEnvSSHHostVar = "DEVSY_PROVIDER_SSH_HOST" +) + var inheritFromEnvironmentTestCases = []assignmentTestCase{ { Name: "assigned, not in the environment", Names: []string{ - "HOST", + testEnvHostName, }, Assignments: []string{ - "HOST=box", + testEnvHostAssign, }, - EnvironmentVariablePrefix: "DEVSY_PROVIDER_SSH_", + EnvironmentVariablePrefix: testEnvSSHPrefix, NotInEnvironment: []string{ - "DEVSY_PROVIDER_SSH_HOST", + testEnvSSHHostVar, }, Environment: map[string]string{}, ExpectedAssignments: []string{ - "HOST=box", + testEnvHostAssign, }, }, { Name: "not assigned, not in the environment", Names: []string{ - "HOST", + testEnvHostName, }, Assignments: []string{}, - EnvironmentVariablePrefix: "DEVSY_PROVIDER_SSH_", + EnvironmentVariablePrefix: testEnvSSHPrefix, NotInEnvironment: []string{ - "DEVSY_PROVIDER_SSH_HOST", + testEnvSSHHostVar, }, Environment: map[string]string{}, ExpectedAssignments: []string{}, @@ -54,30 +60,30 @@ var inheritFromEnvironmentTestCases = []assignmentTestCase{ { Name: "assigned, in the environment", Names: []string{ - "HOST", + testEnvHostName, }, Assignments: []string{ - "HOST=box", + testEnvHostAssign, }, - EnvironmentVariablePrefix: "DEVSY_PROVIDER_SSH_", + EnvironmentVariablePrefix: testEnvSSHPrefix, NotInEnvironment: []string{}, Environment: map[string]string{ - "DEVSY_PROVIDER_SSH_HOST": "another-box", + testEnvSSHHostVar: "another-box", }, ExpectedAssignments: []string{ - "HOST=box", + testEnvHostAssign, }, }, { Name: "not assigned, in the environment", Names: []string{ - "HOST", + testEnvHostName, }, Assignments: []string{}, - EnvironmentVariablePrefix: "DEVSY_PROVIDER_SSH_", + EnvironmentVariablePrefix: testEnvSSHPrefix, NotInEnvironment: []string{}, Environment: map[string]string{ - "DEVSY_PROVIDER_SSH_HOST": "another-box", + testEnvSSHHostVar: "another-box", }, ExpectedAssignments: []string{ "HOST=another-box", @@ -94,8 +100,6 @@ func TestInheritFromEnvironment(t *testing.T) { } func runInheritFromEnvironmentTestCase(t *testing.T, testCase assignmentTestCase) { - fmt.Println(testCase.Name) - for _, k := range testCase.NotInEnvironment { err := os.Unsetenv(k) if err != nil { diff --git a/pkg/options/resolve.go b/pkg/options/resolve.go index e82dd43a4..6f9abd5f5 100644 --- a/pkg/options/resolve.go +++ b/pkg/options/resolve.go @@ -202,13 +202,19 @@ func ResolveOptions( return devConfig, nil } - return applyResolvedProviderOptions( - devConfig, - providerConfig.Name, - resolvedOptionValues, - dynamicOptionDefinitions, - singleMachine, - ), nil + return applyResolvedProviderOptions(devConfig, applyResolvedProviderOptionsParams{ + providerName: providerConfig.Name, + resolvedOptionValues: resolvedOptionValues, + dynamicOptionDefinitions: dynamicOptionDefinitions, + singleMachine: singleMachine, + }), nil +} + +type applyResolvedProviderOptionsParams struct { + providerName string + resolvedOptionValues map[string]config.OptionValue + dynamicOptionDefinitions config.OptionDefinitions + singleMachine *bool } // applyResolvedProviderOptions clones devConfig and records the resolved @@ -216,27 +222,24 @@ func ResolveOptions( // the given provider. func applyResolvedProviderOptions( devConfig *config.Config, - providerName string, - resolvedOptionValues map[string]config.OptionValue, - dynamicOptionDefinitions config.OptionDefinitions, - singleMachine *bool, + p applyResolvedProviderOptionsParams, ) *config.Config { devConfig = config.CloneConfig(devConfig) if devConfig.Current().Providers == nil { devConfig.Current().Providers = map[string]*config.ProviderConfig{} } - if devConfig.Current().Providers[providerName] == nil { - devConfig.Current().Providers[providerName] = &config.ProviderConfig{} + if devConfig.Current().Providers[p.providerName] == nil { + devConfig.Current().Providers[p.providerName] = &config.ProviderConfig{} } - providerCfg := devConfig.Current().Providers[providerName] + providerCfg := devConfig.Current().Providers[p.providerName] providerCfg.Options = map[string]config.OptionValue{} - maps.Copy(providerCfg.Options, resolvedOptionValues) + maps.Copy(providerCfg.Options, p.resolvedOptionValues) providerCfg.DynamicOptions = config.OptionDefinitions{} - maps.Copy(providerCfg.DynamicOptions, dynamicOptionDefinitions) - if singleMachine != nil { - providerCfg.SingleMachine = *singleMachine + maps.Copy(providerCfg.DynamicOptions, p.dynamicOptionDefinitions) + if p.singleMachine != nil { + providerCfg.SingleMachine = *p.singleMachine } return devConfig diff --git a/pkg/options/resolve_test.go b/pkg/options/resolve_test.go index 18d7f1a94..abb0f5dbf 100644 --- a/pkg/options/resolve_test.go +++ b/pkg/options/resolve_test.go @@ -31,61 +31,90 @@ type testCase struct { ExpectedDynamicOptions config.OptionDefinitions } +const ( + testOptTest = "TEST" + testValTest = "test" + testOptCommand = "COMMAND" + testCmdEchoBar = "echo bar" + testValBar = "bar" + testOptCmd1 = "COMMAND1" + testOptCmd2 = "COMMAND2" + testValFoo = "foo" + testOptExpire = "EXPIRE" + testOptNoExp = "NOTEXPIRE" + testOptParent = "PARENT" + testOptChild1 = "CHILD1" + testOptChild2 = "CHILD2" + testRefParent = "${PARENT}" + testOptTest2 = "TEST2" + testValTest2 = "test2" + testOptFoo = "FOO" + testValTest5 = "test5" + testValTest3 = "test3" + testValTest4 = "test4" + testOptTest3 = "TEST3" + testValTest1 = "test1" + testOptTest4 = "TEST4" + testRefChain34 = "${TEST3}-${FOO}-4" + testRefChain24 = "${TEST2}-${FOO}-4" + testOptTest5 = "TEST5" +) + var resolveOptionsTestCases = []testCase{ { Name: "simple", ExtraValues: map[string]string{ - "WORKSPACE_ID": "test", + "WORKSPACE_ID": testValTest, }, ProviderOptions: map[string]*types.Option{ - "TEST": { + testOptTest: { Default: "${WORKSPACE_ID}-test", }, }, ExpectedOptions: map[string]string{ - "TEST": "test-test", + testOptTest: "test-test", }, }, { Name: "dependency", ExtraValues: map[string]string{ - "WORKSPACE_ID": "test", + "WORKSPACE_ID": testValTest, }, ProviderOptions: map[string]*types.Option{ - "TEST": { + testOptTest: { Default: "${WORKSPACE_ID}-test-${COMMAND}-$COMMAND", }, - "COMMAND": { - Command: "echo bar", + testOptCommand: { + Command: testCmdEchoBar, }, }, ExpectedOptions: map[string]string{ - "TEST": "test-test-bar-bar", - "COMMAND": "bar", + testOptTest: "test-test-bar-bar", + testOptCommand: testValBar, }, }, { Name: "No extra values", ProviderOptions: map[string]*types.Option{ - "COMMAND1": { + testOptCmd1: { Command: "echo ${COMMAND2}-test", }, - "COMMAND2": { - Command: "echo bar", + testOptCmd2: { + Command: testCmdEchoBar, }, }, ExpectedOptions: map[string]string{ - "COMMAND1": "bar-test", - "COMMAND2": "bar", + testOptCmd1: "bar-test", + testOptCmd2: testValBar, }, }, { Name: "Cyclic dep", ProviderOptions: map[string]*types.Option{ - "COMMAND1": { + testOptCmd1: { Command: "echo ${COMMAND2}", }, - "COMMAND2": { + testOptCmd2: { Command: "echo ${COMMAND1}", }, }, @@ -94,33 +123,33 @@ var resolveOptionsTestCases = []testCase{ { Name: "Override", ResolvedValues: map[string]config.OptionValue{ - "COMMAND": { - Value: "foo", + testOptCommand: { + Value: testValFoo, UserProvided: true, }, }, ProviderOptions: map[string]*types.Option{ - "COMMAND": { - Command: "echo bar", + testOptCommand: { + Command: testCmdEchoBar, }, }, ExpectedOptions: map[string]string{ - "COMMAND": "foo", + testOptCommand: testValFoo, }, }, { Name: "Override", ResolvedValues: map[string]config.OptionValue{ - "COMMAND": { - Value: "foo", + testOptCommand: { + Value: testValFoo, UserProvided: true, }, }, ProviderOptions: map[string]*types.Option{ - "COMMAND": { - Command: "echo bar", + testOptCommand: { + Command: testCmdEchoBar, }, - "COMMAND1": { + testOptCmd1: { Command: "echo ${COMMAND}-foo-${UNDEFINED}", }, "DEFAULT1": { @@ -128,36 +157,36 @@ var resolveOptionsTestCases = []testCase{ }, }, ExpectedOptions: map[string]string{ - "COMMAND": "foo", - "COMMAND1": "foo-foo-", - "DEFAULT1": "foo-foo-${UNDEFINED}", + testOptCommand: testValFoo, + testOptCmd1: "foo-foo-", + "DEFAULT1": "foo-foo-${UNDEFINED}", }, }, { Name: "Expire", ResolvedValues: map[string]config.OptionValue{ - "EXPIRE": { - Value: "foo", + testOptExpire: { + Value: testValFoo, Filled: &[]types.Time{types.NewTime(time.Time{})}[0], }, - "NOTEXPIRE": { - Value: "foo", + testOptNoExp: { + Value: testValFoo, Filled: &[]types.Time{types.Now()}[0], }, }, ProviderOptions: map[string]*types.Option{ - "EXPIRE": { - Command: "echo bar", + testOptExpire: { + Command: testCmdEchoBar, Cache: "10m", }, - "NOTEXPIRE": { - Command: "echo bar", + testOptNoExp: { + Command: testCmdEchoBar, Cache: "10m", }, }, ExpectedOptions: map[string]string{ - "EXPIRE": "bar", - "NOTEXPIRE": "foo", + testOptExpire: testValBar, + testOptNoExp: testValFoo, }, }, { @@ -168,50 +197,50 @@ var resolveOptionsTestCases = []testCase{ }, }, ExpectedOptions: map[string]string{ - "SELF": "test", + "SELF": testValTest, }, }, { Name: "Recompute children", UserValues: map[string]string{ - "PARENT": "foo", + testOptParent: testValFoo, }, ResolvedValues: map[string]config.OptionValue{ - "PARENT": { - Value: "test", + testOptParent: { + Value: testValTest, UserProvided: true, }, - "CHILD1": { + testOptChild1: { Value: "test-child1", }, - "CHILD2": { + testOptChild2: { Value: "test-child2", }, }, ProviderOptions: map[string]*types.Option{ - "PARENT": {}, - "CHILD1": { + testOptParent: {}, + testOptChild1: { Command: "echo ${PARENT}-child1", }, - "CHILD2": { + testOptChild2: { Default: "${PARENT}-child2", }, }, ExpectedOptions: map[string]string{ - "PARENT": "foo", - "CHILD1": "foo-child1", - "CHILD2": "foo-child2", + testOptParent: testValFoo, + testOptChild1: "foo-child1", + testOptChild2: "foo-child2", }, }, { Name: "Error local global", ProviderOptions: map[string]*types.Option{ - "PARENT": { - Default: "test", + testOptParent: { + Default: testValTest, }, - "CHILD1": { + testOptChild1: { Global: true, - Default: "${PARENT}", + Default: testRefParent, }, }, ExpectErr: true, @@ -219,12 +248,12 @@ var resolveOptionsTestCases = []testCase{ { Name: "Error local var", ProviderOptions: map[string]*types.Option{ - "PARENT": { + testOptParent: { Local: true, - Default: "test", + Default: testValTest, }, - "CHILD1": { - Default: "${PARENT}", + testOptChild1: { + Default: testRefParent, }, }, ExpectErr: true, @@ -232,165 +261,169 @@ var resolveOptionsTestCases = []testCase{ { Name: "Don't resolve local", ProviderOptions: map[string]*types.Option{ - "PARENT": { - Default: "test", + testOptParent: { + Default: testValTest, }, - "CHILD1": { - Default: "${PARENT}", + testOptChild1: { + Default: testRefParent, Local: true, }, }, DontResolveLocal: true, ExpectedOptions: map[string]string{ - "PARENT": "test", + testOptParent: testValTest, }, }, { Name: "Resolve", ProviderOptions: map[string]*types.Option{ - "PARENT": { - Default: "test", + testOptParent: { + Default: testValTest, }, - "CHILD1": { - Default: "${PARENT}", + testOptChild1: { + Default: testRefParent, }, }, DontResolveLocal: true, ExpectedOptions: map[string]string{ - "PARENT": "test", - "CHILD1": "test", + testOptParent: testValTest, + testOptChild1: testValTest, }, }, { Name: "Skip Required", ProviderOptions: map[string]*types.Option{ - "PARENT": { + testOptParent: { Required: true, }, - "CHILD1": { - Default: "${PARENT}", + testOptChild1: { + Default: testRefParent, }, "PARENT2": { Required: true, - Default: "test", + Default: testValTest, }, - "CHILD2": { + testOptChild2: { Default: "${PARENT2}", }, }, SkipRequired: true, ExpectedOptions: map[string]string{ - "PARENT2": "test", - "CHILD2": "test", + "PARENT2": testValTest, + testOptChild2: testValTest, }, }, { Name: "Nested dynamic options", ProviderOptions: map[string]*types.Option{ - "TEST": { - Default: "test", + testOptTest: { + Default: testValTest, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST2": &types.Option{ - Default: "test2", + testOptTest2: &types.Option{ + Default: testValTest2, }, }), }, - "FOO": {Command: "echo bar"}, + testOptFoo: {Command: testCmdEchoBar}, }, ExpectedOptions: map[string]string{ - "TEST": "test", - "TEST2": "test2", - "FOO": "bar", + testOptTest: testValTest, + testOptTest2: testValTest2, + testOptFoo: testValBar, }, ExpectedDynamicOptions: config.OptionDefinitions{ - "TEST2": &types.Option{ - Default: "test2", + testOptTest2: &types.Option{ + Default: testValTest2, }, }, }, { Name: "Dynamic options don't update", ProviderOptions: map[string]*types.Option{ - "TEST": { - Default: "test", + testOptTest: { + Default: testValTest, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST2": &types.Option{ - Default: "test2", + testOptTest2: &types.Option{ + Default: testValTest2, }, }), }, - "FOO": {Command: "echo bar"}, + testOptFoo: {Command: testCmdEchoBar}, }, ResolvedDynamicDefinitions: map[string]*types.Option{ - "TEST2": { - Default: "test5", + testOptTest2: { + Default: testValTest5, }, }, ResolvedValues: map[string]config.OptionValue{ - "TEST": {Value: "test3", Children: []string{"TEST2"}, UserProvided: true}, - "TEST2": {Value: "test4", UserProvided: true}, + testOptTest: { + Value: testValTest3, + Children: []string{testOptTest2}, + UserProvided: true, + }, + testOptTest2: {Value: testValTest4, UserProvided: true}, }, ExpectedOptions: map[string]string{ - "TEST": "test3", - "TEST2": "test4", - "FOO": "bar", + testOptTest: testValTest3, + testOptTest2: testValTest4, + testOptFoo: testValBar, }, ExpectedDynamicOptions: config.OptionDefinitions{ - "TEST2": { - Default: "test2", + testOptTest2: { + Default: testValTest2, }, }, }, { Name: "Dynamic options update", ProviderOptions: map[string]*types.Option{ - "TEST": { - Default: "test", + testOptTest: { + Default: testValTest, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST3": &types.Option{ - Default: "test2", + testOptTest3: &types.Option{ + Default: testValTest2, }, }), }, - "FOO": {Command: "echo bar"}, + testOptFoo: {Command: testCmdEchoBar}, }, UserValues: map[string]string{ - "TEST": "test1", + testOptTest: testValTest1, }, ResolvedValues: map[string]config.OptionValue{ - "TEST": {Value: "test3", Children: []string{"TEST2"}}, - "TEST2": {Value: "test4"}, + testOptTest: {Value: testValTest3, Children: []string{testOptTest2}}, + testOptTest2: {Value: testValTest4}, }, ResolvedDynamicDefinitions: map[string]*types.Option{ - "TEST2": { - Default: "test5", + testOptTest2: { + Default: testValTest5, }, }, ExpectedOptions: map[string]string{ - "TEST": "test1", - "TEST3": "test2", - "FOO": "bar", + testOptTest: testValTest1, + testOptTest3: testValTest2, + testOptFoo: testValBar, }, ExpectedDynamicOptions: config.OptionDefinitions{ - "TEST3": &types.Option{ - Default: "test2", + testOptTest3: &types.Option{ + Default: testValTest2, }, }, }, { Name: "Nested dynamic options", ProviderOptions: map[string]*types.Option{ - "TEST": { - Default: "test1", + testOptTest: { + Default: testValTest1, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST2": &types.Option{ - Default: "test2", + testOptTest2: &types.Option{ + Default: testValTest2, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST3": &types.Option{ - Default: "test3", + testOptTest3: &types.Option{ + Default: testValTest3, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST4": &types.Option{ - Default: "${TEST3}-${FOO}-4", + testOptTest4: &types.Option{ + Default: testRefChain34, }, }), }, @@ -398,56 +431,56 @@ var resolveOptionsTestCases = []testCase{ }, }), }, - "FOO": {Command: "echo bar"}, + testOptFoo: {Command: testCmdEchoBar}, }, ExpectedOptions: map[string]string{ - "TEST": "test1", - "TEST2": "test2", - "TEST3": "test3", - "TEST4": "test3-bar-4", - "FOO": "bar", + testOptTest: testValTest1, + testOptTest2: testValTest2, + testOptTest3: testValTest3, + testOptTest4: "test3-bar-4", + testOptFoo: testValBar, }, ExpectedDynamicOptions: config.OptionDefinitions{ - "TEST2": &types.Option{ - Default: "test2", + testOptTest2: &types.Option{ + Default: testValTest2, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST3": &types.Option{ - Default: "test3", + testOptTest3: &types.Option{ + Default: testValTest3, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST4": &types.Option{ - Default: "${TEST3}-${FOO}-4", + testOptTest4: &types.Option{ + Default: testRefChain34, }, }), }, }), }, - "TEST3": &types.Option{ - Default: "test3", + testOptTest3: &types.Option{ + Default: testValTest3, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST4": &types.Option{ - Default: "${TEST3}-${FOO}-4", + testOptTest4: &types.Option{ + Default: testRefChain34, }, }), }, - "TEST4": &types.Option{ - Default: "${TEST3}-${FOO}-4", + testOptTest4: &types.Option{ + Default: testRefChain34, }, }, }, { Name: "Nested dynamic options skip required", ProviderOptions: map[string]*types.Option{ - "TEST": { - Default: "test1", + testOptTest: { + Default: testValTest1, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST2": &types.Option{ + testOptTest2: &types.Option{ Required: true, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST3": &types.Option{ - Default: "test3", + testOptTest3: &types.Option{ + Default: testValTest3, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST4": &types.Option{ - Default: "${TEST3}-${FOO}-4", + testOptTest4: &types.Option{ + Default: testRefChain34, }, }), }, @@ -455,22 +488,22 @@ var resolveOptionsTestCases = []testCase{ }, }), }, - "FOO": {Command: "echo bar"}, + testOptFoo: {Command: testCmdEchoBar}, }, SkipRequired: true, ExpectedOptions: map[string]string{ - "TEST": "test1", - "FOO": "bar", + testOptTest: testValTest1, + testOptFoo: testValBar, }, ExpectedDynamicOptions: config.OptionDefinitions{ - "TEST2": &types.Option{ + testOptTest2: &types.Option{ Required: true, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST3": &types.Option{ - Default: "test3", + testOptTest3: &types.Option{ + Default: testValTest3, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST4": &types.Option{ - Default: "${TEST3}-${FOO}-4", + testOptTest4: &types.Option{ + Default: testRefChain34, }, }), }, @@ -481,17 +514,17 @@ var resolveOptionsTestCases = []testCase{ { Name: "Nested dynamic options use option", ProviderOptions: map[string]*types.Option{ - "TEST": { - Default: "test1", + testOptTest: { + Default: testValTest1, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST2": &types.Option{ + testOptTest2: &types.Option{ Required: true, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST3": &types.Option{ - Default: "test3", + testOptTest3: &types.Option{ + Default: testValTest3, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST4": &types.Option{ - Default: "${TEST2}-${FOO}-4", + testOptTest4: &types.Option{ + Default: testRefChain24, }, }), }, @@ -499,157 +532,157 @@ var resolveOptionsTestCases = []testCase{ }, }), }, - "FOO": {Command: "echo bar"}, + testOptFoo: {Command: testCmdEchoBar}, }, SkipRequired: true, UserValues: map[string]string{ - "TEST2": "test2", + testOptTest2: testValTest2, }, ExpectedOptions: map[string]string{ - "TEST": "test1", - "TEST2": "test2", - "TEST3": "test3", - "TEST4": "test2-bar-4", - "FOO": "bar", + testOptTest: testValTest1, + testOptTest2: testValTest2, + testOptTest3: testValTest3, + testOptTest4: "test2-bar-4", + testOptFoo: testValBar, }, ExpectedDynamicOptions: config.OptionDefinitions{ - "TEST2": &types.Option{ + testOptTest2: &types.Option{ Required: true, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST3": &types.Option{ - Default: "test3", + testOptTest3: &types.Option{ + Default: testValTest3, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST4": &types.Option{ - Default: "${TEST2}-${FOO}-4", + testOptTest4: &types.Option{ + Default: testRefChain24, }, }), }, }), }, - "TEST3": &types.Option{ - Default: "test3", + testOptTest3: &types.Option{ + Default: testValTest3, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST4": &types.Option{ - Default: "${TEST2}-${FOO}-4", + testOptTest4: &types.Option{ + Default: testRefChain24, }, }), }, - "TEST4": &types.Option{ - Default: "${TEST2}-${FOO}-4", + testOptTest4: &types.Option{ + Default: testRefChain24, }, }, }, { Name: "Nested dynamic options use option", ProviderOptions: map[string]*types.Option{ - "TEST": { - Default: "test1", + testOptTest: { + Default: testValTest1, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST2": &types.Option{ - Default: "test2", + testOptTest2: &types.Option{ + Default: testValTest2, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST3": &types.Option{ - Default: "test3", + testOptTest3: &types.Option{ + Default: testValTest3, }, }), }, }), }, - "FOO": {Command: "echo bar"}, + testOptFoo: {Command: testCmdEchoBar}, }, ResolvedValues: map[string]config.OptionValue{ - "TEST5": { - Value: "test5", + testOptTest5: { + Value: testValTest5, }, }, ExpectedOptions: map[string]string{ - "TEST": "test1", - "TEST2": "test2", - "TEST3": "test3", - "FOO": "bar", + testOptTest: testValTest1, + testOptTest2: testValTest2, + testOptTest3: testValTest3, + testOptFoo: testValBar, }, ExpectedDynamicOptions: config.OptionDefinitions{ - "TEST2": &types.Option{ - Default: "test2", + testOptTest2: &types.Option{ + Default: testValTest2, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST3": &types.Option{ - Default: "test3", + testOptTest3: &types.Option{ + Default: testValTest3, }, }), }, - "TEST3": &types.Option{ - Default: "test3", + testOptTest3: &types.Option{ + Default: testValTest3, }, }, }, { Name: "Dynamic options unused option", ProviderOptions: map[string]*types.Option{ - "TEST": { - Default: "test1", + testOptTest: { + Default: testValTest1, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST2": &types.Option{ - Default: "test2", + testOptTest2: &types.Option{ + Default: testValTest2, }, }), }, - "FOO": {Command: "echo bar"}, + testOptFoo: {Command: testCmdEchoBar}, }, ResolvedValues: map[string]config.OptionValue{ - "TEST5": { - Value: "test5", + testOptTest5: { + Value: testValTest5, }, }, ResolvedDynamicDefinitions: map[string]*types.Option{ - "TEST5": { - Default: "test2", + testOptTest5: { + Default: testValTest2, }, }, ExpectedOptions: map[string]string{ - "TEST": "test1", - "TEST2": "test2", - "FOO": "bar", + testOptTest: testValTest1, + testOptTest2: testValTest2, + testOptFoo: testValBar, }, ExpectedDynamicOptions: config.OptionDefinitions{ - "TEST2": &types.Option{ - Default: "test2", + testOptTest2: &types.Option{ + Default: testValTest2, }, }, }, { Name: "Dynamic options update default", ProviderOptions: map[string]*types.Option{ - "TEST": { - Default: "test1", + testOptTest: { + Default: testValTest1, SubOptionsCommand: optionsToSubCommand(config.OptionDefinitions{ - "TEST2": &types.Option{ - Default: "test3", + testOptTest2: &types.Option{ + Default: testValTest3, }, }), }, - "FOO": {Command: "echo bar"}, + testOptFoo: {Command: testCmdEchoBar}, }, ResolvedValues: map[string]config.OptionValue{ - "TEST": { - Value: "test1", + testOptTest: { + Value: testValTest1, }, - "TEST2": { - Value: "test2", + testOptTest2: { + Value: testValTest2, }, }, ResolvedDynamicDefinitions: map[string]*types.Option{ - "TEST2": { - Default: "test2", + testOptTest2: { + Default: testValTest2, }, }, ExpectedOptions: map[string]string{ - "TEST": "test1", - "TEST2": "test3", - "FOO": "bar", + testOptTest: testValTest1, + testOptTest2: testValTest3, + testOptFoo: testValBar, }, ExpectedDynamicOptions: config.OptionDefinitions{ - "TEST2": &types.Option{ - Default: "test3", + testOptTest2: &types.Option{ + Default: testValTest3, }, }, }, diff --git a/pkg/platform/form/form.go b/pkg/platform/form/form.go index d752ca4dd..d9e203bce 100644 --- a/pkg/platform/form/form.go +++ b/pkg/platform/form/form.go @@ -31,27 +31,36 @@ func CreateInstance( formCtx, cancelForm := context.WithCancel(ctx) defer cancelForm() - selectedProject, selectedTemplate, selectedTemplateVersion, err := runCreateSelectionForm( - ctx, baseClient, formCtx, cancelForm, - ) + selection, err := runCreateSelectionForm(ctx, baseClient, formCtx, cancelForm) if err != nil { return nil, err } renderedParameters, err := renderedParametersForCreate( formCtx, - selectedTemplate, - selectedTemplateVersion, + selection.template, + selection.templateVersion, ) if err != nil { return nil, err } - return buildCreatedInstance( - id, uid, source, picture, - selectedProject, selectedTemplate, selectedTemplateVersion, - renderedParameters, - ), nil + return buildCreatedInstance(buildCreatedInstanceParams{ + id: id, + uid: uid, + source: source, + picture: picture, + selectedProject: selection.project, + selectedTemplate: selection.template, + selectedTemplateVersion: selection.templateVersion, + renderedParameters: renderedParameters, + }), nil +} + +type createSelection struct { + project *managementv1.Project + template *managementv1.DevsyWorkspaceTemplate + templateVersion string } func runCreateSelectionForm( @@ -59,14 +68,14 @@ func runCreateSelectionForm( baseClient client.Client, formCtx context.Context, cancelForm CancelFunc, -) (*managementv1.Project, *managementv1.DevsyWorkspaceTemplate, string, error) { +) (createSelection, error) { var selectedCluster *managementv1.Cluster var selectedProject *managementv1.Project var selectedTemplate *managementv1.DevsyWorkspaceTemplate selectedTemplateVersion := "" projectOptions, err := projectOptions(ctx, baseClient) if err != nil { - return nil, nil, "", err + return createSelection{}, err } err = huh.NewForm( huh.NewGroup( @@ -97,10 +106,14 @@ func runCreateSelectionForm( ), ).RunWithContext(formCtx) if err != nil { - return nil, nil, "", err + return createSelection{}, err } - return selectedProject, selectedTemplate, selectedTemplateVersion, nil + return createSelection{ + project: selectedProject, + template: selectedTemplate, + templateVersion: selectedTemplateVersion, + }, nil } func renderedParametersForCreate( @@ -120,35 +133,37 @@ func renderedParametersForCreate( return runParameterForm(formCtx, fieldParameters) } -func buildCreatedInstance( - id, uid, source, picture string, - selectedProject *managementv1.Project, - selectedTemplate *managementv1.DevsyWorkspaceTemplate, - selectedTemplateVersion string, - renderedParameters string, -) *managementv1.DevsyWorkspaceInstance { +type buildCreatedInstanceParams struct { + id, uid, source, picture string + selectedProject *managementv1.Project + selectedTemplate *managementv1.DevsyWorkspaceTemplate + selectedTemplateVersion string + renderedParameters string +} + +func buildCreatedInstance(p buildCreatedInstanceParams) *managementv1.DevsyWorkspaceInstance { return &managementv1.DevsyWorkspaceInstance{ ObjectMeta: metav1.ObjectMeta{ - GenerateName: encoding.SafeConcatNameMax([]string{id}, 53) + "-", - Namespace: project.ProjectNamespace(selectedProject.GetName()), + GenerateName: encoding.SafeConcatNameMax([]string{p.id}, 53) + "-", + Namespace: project.ProjectNamespace(p.selectedProject.GetName()), Labels: map[string]string{ - storagev1.DevsyWorkspaceIDLabel: id, - storagev1.DevsyWorkspaceUIDLabel: uid, - config.K8sProjectLabel: selectedProject.GetName(), + storagev1.DevsyWorkspaceIDLabel: p.id, + storagev1.DevsyWorkspaceUIDLabel: p.uid, + config.K8sProjectLabel: p.selectedProject.GetName(), }, Annotations: map[string]string{ - storagev1.DevsyWorkspacePictureAnnotation: picture, - storagev1.DevsyWorkspaceSourceAnnotation: source, + storagev1.DevsyWorkspacePictureAnnotation: p.picture, + storagev1.DevsyWorkspaceSourceAnnotation: p.source, }, }, Spec: managementv1.DevsyWorkspaceInstanceSpec{ DevsyWorkspaceInstanceSpec: storagev1.DevsyWorkspaceInstanceSpec{ - DisplayName: id, + DisplayName: p.id, TemplateRef: &storagev1.TemplateRef{ - Name: selectedTemplate.GetName(), - Version: selectedTemplateVersion, + Name: p.selectedTemplate.GetName(), + Version: p.selectedTemplateVersion, }, - Parameters: renderedParameters, + Parameters: p.renderedParameters, }, }, } diff --git a/pkg/platform/kubeconfig.go b/pkg/platform/kubeconfig.go index 2050b89d9..574d2a14d 100644 --- a/pkg/platform/kubeconfig.go +++ b/pkg/platform/kubeconfig.go @@ -273,21 +273,19 @@ func kubeConfigForVirtualClusterInstance( return nil, fmt.Errorf("get virtual cluster instance: %w", err) } - req := newVClusterKubeConfigRequest( - ctx, - managementClient, - namespace, - projectName, - virtualClusterInstance, - ) + req := newVClusterKubeConfigRequest(ctx, newVClusterKubeConfigRequestParams{ + managementClient: managementClient, + namespace: namespace, + projectName: projectName, + virtualClusterInstance: virtualClusterInstance, + }) - cfg, handled, err := directVirtualClusterKubeConfig( - ctx, - baseClient, - projectName, - virtualClusterInstance, - req, - ) + cfg, handled, err := directVirtualClusterKubeConfig(ctx, directVirtualClusterKubeConfigParams{ + baseClient: baseClient, + projectName: projectName, + virtualClusterInstance: virtualClusterInstance, + req: req, + }) if handled || err != nil { return cfg, err } @@ -311,56 +309,65 @@ func kubeConfigForVirtualClusterInstance( }) } +type newVClusterKubeConfigRequestParams struct { + managementClient kube.Interface + namespace string + projectName string + virtualClusterInstance *managementv1.VirtualClusterInstance +} + // newVClusterKubeConfigRequest builds the scoped request shared by the // direct-ingress, direct-cluster-endpoint, and access-key kube config paths. func newVClusterKubeConfigRequest( ctx context.Context, - managementClient kube.Interface, - namespace, projectName string, - virtualClusterInstance *managementv1.VirtualClusterInstance, + p newVClusterKubeConfigRequestParams, ) vClusterKubeConfigRequest { scope := &storagev1.AccessKeyScope{ VirtualClusters: []storagev1.AccessKeyScopeVirtualCluster{{ - Project: projectName, - VirtualCluster: virtualClusterInstance.Name, + Project: p.projectName, + VirtualCluster: p.virtualClusterInstance.Name, }}, } return vClusterKubeConfigRequest{ ctx: ctx, - managementClient: managementClient, - namespace: namespace, - projectName: projectName, + managementClient: p.managementClient, + namespace: p.namespace, + projectName: p.projectName, scope: scope, ttl: int64(configTTL.Seconds()), - instance: virtualClusterInstance, + instance: p.virtualClusterInstance, } } +type directVirtualClusterKubeConfigParams struct { + baseClient client.Client + projectName string + virtualClusterInstance *managementv1.VirtualClusterInstance + req vClusterKubeConfigRequest +} + // directVirtualClusterKubeConfig resolves a kube config via direct ingress // or a direct cluster endpoint, if either is available for this virtual // cluster. handled reports whether one of those paths applied; if not, the // caller should fall back to access-key-based config. func directVirtualClusterKubeConfig( ctx context.Context, - baseClient client.Client, - projectName string, - virtualClusterInstance *managementv1.VirtualClusterInstance, - req vClusterKubeConfigRequest, + p directVirtualClusterKubeConfigParams, ) (cfg *clientcmdapi.Config, handled bool, err error) { // direct virtual cluster ingress access? - virtualCluster := virtualClusterInstance.Status.VirtualCluster + virtualCluster := p.virtualClusterInstance.Status.VirtualCluster if virtualCluster != nil && virtualCluster.AccessPoint.Ingress.Enabled { - cfg, err = directIngressKubeConfig(req) + cfg, err = directIngressKubeConfig(p.req) return cfg, true, err } // find cluster by clusterRef hostCluster, err := findHostCluster( ctx, - baseClient, - projectName, - virtualClusterInstance.Spec.ClusterRef.ClusterRef, + p.baseClient, + p.projectName, + p.virtualClusterInstance.Spec.ClusterRef.ClusterRef, ) if err != nil { return nil, true, fmt.Errorf("find host cluster: %w", err) @@ -368,7 +375,7 @@ func directVirtualClusterKubeConfig( // direct cluster access? if hostCluster.GetAnnotations()[annotations.LoftDirectClusterEndpoint] != "" { - cfg, err = directClusterEndpointKubeConfig(req, hostCluster) + cfg, err = directClusterEndpointKubeConfig(p.req, hostCluster) return cfg, true, err } diff --git a/pkg/ssh/config_test.go b/pkg/ssh/config_test.go index 5330f66a4..1c4ac9de9 100644 --- a/pkg/ssh/config_test.go +++ b/pkg/ssh/config_test.go @@ -33,11 +33,11 @@ var addHostSectionTestCases = []struct { { name: "Basic host addition", config: "", - execPath: "/path/to/exec", - host: "testhost", - user: "testuser", - context: "testcontext", - workspace: "testworkspace", + execPath: testExecPath, + host: testHostBasic, + user: testUser, + context: testContextAlt, + workspace: testWorkspaceAlt, workdir: "", command: "", gpgagent: false, @@ -57,11 +57,11 @@ Host testhost { name: "AWS provider with ConnectTimeout", config: "", - execPath: "/path/to/exec", - host: "testhost", - user: "testuser", - context: "testcontext", - workspace: "testworkspace", + execPath: testExecPath, + host: testHostBasic, + user: testUser, + context: testContextAlt, + workspace: testWorkspaceAlt, workdir: "", command: "", gpgagent: false, @@ -82,11 +82,11 @@ Host testhost { name: "Basic host addition with DEVSY_HOME", config: "", - execPath: "/path/to/exec", - host: "testhost", - user: "testuser", - context: "testcontext", - workspace: "testworkspace", + execPath: testExecPath, + host: testHostBasic, + user: testUser, + context: testContextAlt, + workspace: testWorkspaceAlt, workdir: "", command: "", gpgagent: false, @@ -107,11 +107,11 @@ Host testhost { name: "Host addition with workdir", config: "", - execPath: "/path/to/exec", - host: "testhost", - user: "testuser", - context: "testcontext", - workspace: "testworkspace", + execPath: testExecPath, + host: testHostBasic, + user: testUser, + context: testContextAlt, + workspace: testWorkspaceAlt, workdir: "/path/to/workdir", command: "", gpgagent: false, @@ -132,11 +132,11 @@ Host testhost { name: "Host addition with gpg agent", config: "", - execPath: "/path/to/exec", - host: "testhost", - user: "testuser", - context: "testcontext", - workspace: "testworkspace", + execPath: testExecPath, + host: testHostBasic, + user: testUser, + context: testContextAlt, + workspace: testWorkspaceAlt, workdir: "", command: "", gpgagent: true, @@ -157,11 +157,11 @@ Host testhost { name: "Host addition with custom command", config: "", - execPath: "/path/to/exec", - host: "testhost", - user: "testuser", - context: "testcontext", - workspace: "testworkspace", + execPath: testExecPath, + host: testHostBasic, + user: testUser, + context: testContextAlt, + workspace: testWorkspaceAlt, workdir: "", command: "ssh -W %h:%p bastion", gpgagent: false, @@ -182,11 +182,11 @@ Host testhost name: "Host addition to existing config", config: `Host existinghost User existinguser`, - execPath: "/path/to/exec", - host: "testhost", - user: "testuser", - context: "testcontext", - workspace: "testworkspace", + execPath: testExecPath, + host: testHostBasic, + user: testUser, + context: testContextAlt, + workspace: testWorkspaceAlt, workdir: "", command: "", gpgagent: false, @@ -220,11 +220,11 @@ Host existingtesthost Host existinghost User existinguser`, - execPath: "/path/to/exec", - host: "testhost", - user: "testuser", - context: "testcontext", - workspace: "testworkspace", + execPath: testExecPath, + host: testHostBasic, + user: testUser, + context: testContextAlt, + workspace: testWorkspaceAlt, workdir: "", command: "", gpgagent: false, @@ -263,11 +263,11 @@ Include ~/config2 Include ~/config3`, - execPath: "/path/to/exec", - host: "testhost", - user: "testuser", - context: "testcontext", - workspace: "testworkspace", + execPath: testExecPath, + host: testHostBasic, + user: testUser, + context: testContextAlt, + workspace: testWorkspaceAlt, workdir: "", command: "", gpgagent: false, From c132a2b4fc1352d0abe1ce565a298d1b17977bc4 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 7 Aug 2026 01:39:13 +0000 Subject: [PATCH 5/5] fix(lint): address CodeRabbit review findings on funlen extraction ResolveOptions dereferenced devConfig before its nil guard; move the guard to the top of the function so a nil input returns cleanly instead of panicking. Also stop computing the NOTEXPIRE test timestamp at package-init time (types.Now() in a package-level var), which could make TestResolveOptions flaky if run long after process start; the timestamp is now generated immediately before Resolve runs. --- pkg/options/resolve.go | 8 ++++---- pkg/options/resolve_test.go | 17 +++++++++++++++-- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/pkg/options/resolve.go b/pkg/options/resolve.go index 6f9abd5f5..d10c41d37 100644 --- a/pkg/options/resolve.go +++ b/pkg/options/resolve.go @@ -163,6 +163,10 @@ func ResolveOptions( skipSubOptions bool, singleMachine *bool, ) (*config.Config, error) { + if devConfig == nil { + return nil, nil + } + // get binary paths binaryPaths, err := provider.GetBinaries(devConfig.DefaultContext, providerConfig) if err != nil { @@ -198,10 +202,6 @@ func ResolveOptions( return nil, err } - if devConfig == nil { - return devConfig, nil - } - return applyResolvedProviderOptions(devConfig, applyResolvedProviderOptionsParams{ providerName: providerConfig.Name, resolvedOptionValues: resolvedOptionValues, diff --git a/pkg/options/resolve_test.go b/pkg/options/resolve_test.go index abb0f5dbf..680034689 100644 --- a/pkg/options/resolve_test.go +++ b/pkg/options/resolve_test.go @@ -29,6 +29,9 @@ type testCase struct { ExpectErr bool ExpectedOptions map[string]string ExpectedDynamicOptions config.OptionDefinitions + + // FreshFilledKeys get Filled set to time.Now() right before Resolve runs. + FreshFilledKeys []string } const ( @@ -170,8 +173,7 @@ var resolveOptionsTestCases = []testCase{ Filled: &[]types.Time{types.NewTime(time.Time{})}[0], }, testOptNoExp: { - Value: testValFoo, - Filled: &[]types.Time{types.Now()}[0], + Value: testValFoo, }, }, ProviderOptions: map[string]*types.Option{ @@ -188,6 +190,7 @@ var resolveOptionsTestCases = []testCase{ testOptExpire: testValBar, testOptNoExp: testValFoo, }, + FreshFilledKeys: []string{testOptNoExp}, }, { Name: "Ignore self", @@ -696,8 +699,18 @@ func TestResolveOptions(t *testing.T) { } } +func applyFreshFilledTimestamps(tc testCase) { + for _, key := range tc.FreshFilledKeys { + value := tc.ResolvedValues[key] + now := types.Now() + value.Filled = &now + tc.ResolvedValues[key] = value + } +} + func runResolveTestCase(t *testing.T, tc testCase) { t.Helper() + applyFreshFilledTimestamps(tc) r := resolver.New(tc.UserValues, tc.ExtraValues, buildResolverOpts(tc)...) options, dynamicOptions, err := r.Resolve( context.Background(),