chore: Refactor common server cli components for reusability #7103

Emyrk · 2023-04-12T15:12:06Z

Workspace-proxy cmd needs to redo all of this, better to be shared.

See the workspace-proxy cmd (WIP):

Lines 98 to 131 in a425487

    
           Handler: func(inv *clibase.Invocation) error { 
        
           	scd, err := cli.SetupServerCmd(inv, cfg) 
        
           	if err != nil { 
        
           		return err 
        
           	} 
        
           	defer scd.Close() 
        
           	ctx := scd.Ctx 
        
           	pu, _ := url.Parse("http://localhost:3000") 
        
           	proxy, err := wsproxy.New(&wsproxy.Options{ 
        
           		Logger: scd.Logger, 
        
           		// TODO: PrimaryAccessURL 
        
           		PrimaryAccessURL: pu, 
        
           		AccessURL:        cfg.AccessURL.Value(), 
        
           		AppHostname:      scd.AppHostname, 
        
           		AppHostnameRegex: scd.AppHostnameRegex, 
        
           		RealIPConfig:     scd.RealIPConfig, 
        
           		// TODO: AppSecurityKey 
        
           		AppSecurityKey:     workspaceapps.SecurityKey{}, 
        
           		Tracing:            scd.Tracer, 
        
           		PrometheusRegistry: scd.PrometheusRegistry, 
        
           		APIRateLimit:       int(cfg.RateLimit.API.Value()), 
        
           		SecureAuthCookie:   cfg.SecureAuthCookie.Value(), 
        
           		// TODO: DisablePathApps 
        
           		DisablePathApps: false, 
        
           		// TODO: ProxySessionToken 
        
           		ProxySessionToken: "", 
        
           	}) 
        
           	if err != nil { 
        
           		return xerrors.Errorf("create workspace proxy: %w", err) 
        
           	} 
        
           	shutdownConnsCtx, shutdownConns := context.WithCancel(ctx) 
        
           	defer shutdownConns()

Workspace-proxy cmd needs to redo all of this, better to be shared

kylecarbs · 2023-04-12T22:34:25Z

cli/server.go

+
+// SetupServerCmd sets up the common elements of starting a server daemon.
+// This is used by both coderd and workspace proxies.
+func SetupServerCmd(inv *clibase.Invocation, cfg *codersdk.DeploymentValues) (_ *CommonServerCmd, err error) {


I think this is too broad of a function name. Is there a reason this needs to be abstracted together and can't just be duplicated? I'm having a difficult time understanding why I'd put something here, and whether it would break workspace proxies or not.

I can duplicate it, and maybe that's the way to go. I was just trying not to duplicate all this logic across the Coderd and workspace proxy.

I'm just afraid of regressions when things are not properly done in both places.

My other idea was to refactor this into as many functions as I can so at least the shared code is a bit less LOC

If we don't share the shared code we will start seeing devs do something in server.go and not do it in proxy.go just like we did in V1. Code duplication is off the table IMO because of this.

Just using functions and not a big "CommonServerCmd" struct would also work

I can whip up the making common functions option. It's not as nice since I have to export a lot of things from this cli package, and new things still have be copied into 2 locations.

@kylecarbs I need this PR where I reuse the code, or I need this PR #7118.

The second PR just exports all the functions that I need to make the workspace proxy command act the same.

cli/server.go

Emyrk · 2023-04-13T14:07:32Z

closed for #7118

chore: Refactor common server cli components for reusability

66c1b13

Workspace-proxy cmd needs to redo all of this, better to be shared

github-actions bot assigned Emyrk Apr 12, 2023

Emyrk requested a review from kylecarbs April 12, 2023 15:13

Emyrk added 4 commits April 12, 2023 10:14

Import order

fe70221

make fmt

bd9d9a0

Remove defer close

b6286bc

Fix prometheus register

f12bc59

Emyrk requested a review from deansheather April 12, 2023 19:07

kylecarbs reviewed Apr 12, 2023

View reviewed changes

deansheather approved these changes Apr 13, 2023

View reviewed changes

cli/server.go Show resolved Hide resolved

Close idle connections of the new http client

7f8acbd

Emyrk closed this Apr 13, 2023

Emyrk deleted the stevenmasley/server_cli_shared branch April 17, 2023 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Refactor common server cli components for reusability #7103

chore: Refactor common server cli components for reusability #7103

Emyrk commented Apr 12, 2023

kylecarbs Apr 12, 2023

Emyrk Apr 12, 2023

deansheather Apr 13, 2023

Emyrk Apr 13, 2023

Emyrk Apr 13, 2023

Emyrk commented Apr 13, 2023

	Handler: func(inv *clibase.Invocation) error {
	scd, err := cli.SetupServerCmd(inv, cfg)
	if err != nil {
	return err
	}
	defer scd.Close()
	ctx := scd.Ctx

	pu, _ := url.Parse("http://localhost:3000")
	proxy, err := wsproxy.New(&wsproxy.Options{
	Logger: scd.Logger,
	// TODO: PrimaryAccessURL
	PrimaryAccessURL: pu,
	AccessURL: cfg.AccessURL.Value(),
	AppHostname: scd.AppHostname,
	AppHostnameRegex: scd.AppHostnameRegex,
	RealIPConfig: scd.RealIPConfig,
	// TODO: AppSecurityKey
	AppSecurityKey: workspaceapps.SecurityKey{},
	Tracing: scd.Tracer,
	PrometheusRegistry: scd.PrometheusRegistry,
	APIRateLimit: int(cfg.RateLimit.API.Value()),
	SecureAuthCookie: cfg.SecureAuthCookie.Value(),
	// TODO: DisablePathApps
	DisablePathApps: false,
	// TODO: ProxySessionToken
	ProxySessionToken: "",
	})
	if err != nil {
	return xerrors.Errorf("create workspace proxy: %w", err)
	}

	shutdownConnsCtx, shutdownConns := context.WithCancel(ctx)
	defer shutdownConns()

chore: Refactor common server cli components for reusability #7103

chore: Refactor common server cli components for reusability #7103

Conversation

Emyrk commented Apr 12, 2023

kylecarbs Apr 12, 2023

Choose a reason for hiding this comment

Emyrk Apr 12, 2023

Choose a reason for hiding this comment

deansheather Apr 13, 2023

Choose a reason for hiding this comment

Emyrk Apr 13, 2023

Choose a reason for hiding this comment

Emyrk Apr 13, 2023

Choose a reason for hiding this comment

Emyrk commented Apr 13, 2023