Skip to content
This repository was archived by the owner on Aug 30, 2024. It is now read-only.

Commit e9acc2d

Browse files
creackcreack
committed
Cleanup:
- Scope errors when possible. - Fix nil pointers in unmarshal. - Align struct tags for readability. - Explicitly discard errors with a comment on why. - Import package order. - Fmt verb for types. - More comments. Signed-off-by: Guillaume J. Charmes <guillaume@coder.com>
1 parent 8022bfc commit e9acc2d

File tree

25 files changed

+405
-354
lines changed

25 files changed

+405
-354
lines changed

‎ci/integration/setup_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ func init() {
3232

3333
// build the coder-cli binary and move to the integration testing bin directory
3434
func build(path string) error {
35-
tar := fmt.Sprintf("coder-cli-linux-amd64.tar.gz")
35+
tar := "coder-cli-linux-amd64.tar.gz"
3636
dir := filepath.Dir(path)
3737
cmd := exec.Command(
3838
"sh", "-c",

‎cmd/coder/main.go

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,31 +15,35 @@ import (
1515
"go.coder.com/flog"
1616
)
1717

18-
var (
19-
version string = "unknown"
20-
)
18+
// Using a global for the version so it can be set at build time using ldflags.
19+
var version string = "unknown"
2120

2221
func main() {
2322
ctx, cancel := context.WithCancel(context.Background())
2423
defer cancel()
2524

25+
// If requested, spin up the pprof webserver.
2626
if os.Getenv("PPROF") != "" {
2727
go func() {
2828
log.Println(http.ListenAndServe("localhost:6060", nil))
2929
}()
3030
}
3131

32+
// TODO: Document why this is needed. Only does something on windows.
3233
stdoutState, err := xterminal.MakeOutputRaw(os.Stdout.Fd())
3334
if err != nil {
34-
flog.Fatal("set output to raw: %v", err)
35+
flog.Fatal("set output to raw: %s", err)
3536
}
36-
defer xterminal.Restore(os.Stdout.Fd(), stdoutState)
37+
defer func() {
38+
// Best effort. Would result in broken terminal on window but nothing we can do about it.
39+
_ = xterminal.Restore(os.Stdout.Fd(), stdoutState)
40+
}()
3741

3842
app := cmd.Make()
3943
app.Version = fmt.Sprintf("%s %s %s/%s", version, runtime.Version(), runtime.GOOS, runtime.GOARCH)
4044

41-
err = app.ExecuteContext(ctx)
42-
if err != nil {
45+
if err := app.ExecuteContext(ctx); err != nil {
46+
// NOTE: The cmd lib will handle the logs, no need to do it here as well, simply exit with error code.
4347
os.Exit(1)
4448
}
4549
}

‎coder-sdk/activity.go

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,18 +5,23 @@ import (
55
"net/http"
66
)
77

8+
type activityRequest struct {
9+
Source string `json:"source"`
10+
EnvironmentID string `json:"environment_id"`
11+
}
12+
813
// PushActivity pushes CLI activity to Coder.
9-
func (c Client) PushActivity(ctx context.Context, source string, envID string) error {
10-
res, err := c.request(ctx, http.MethodPost, "/api/metrics/usage/push", map[string]string{
11-
"source": source,
12-
"environment_id": envID,
14+
func (c Client) PushActivity(ctx context.Context, source, envID string) error {
15+
resp, err := c.request(ctx, http.MethodPost, "/api/metrics/usage/push", activityRequest{
16+
Source: source,
17+
EnvironmentID: envID,
1318
})
1419
if err != nil {
1520
return err
1621
}
1722

18-
if res.StatusCode != http.StatusOK {
19-
return bodyError(res)
23+
if resp.StatusCode != http.StatusOK {
24+
return bodyError(resp)
2025
}
2126
return nil
2227
}

‎coder-sdk/client.go

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,8 @@ type Client struct {
1515
Token string
1616
}
1717

18-
func (c Client) copyURL() *url.URL {
19-
swp := *c.BaseURL
20-
return &swp
21-
}
22-
23-
func (c *Client) http() (*http.Client, error) {
18+
// newHTTPClient creates a default undrlying http client and sets the auth cookie.
19+
func (c *Client) newHTTPClient() (*http.Client, error) {
2420
jar, err := cookiejar.New(nil)
2521
if err != nil {
2622
return nil, err
@@ -36,5 +32,7 @@ func (c *Client) http() (*http.Client, error) {
3632
Secure: c.BaseURL.Scheme == "https",
3733
},
3834
})
35+
// TODO: Consider setting a timeout.
36+
// TODO: Consider having a persistent client to allow for single connection.
3937
return &http.Client{Jar: jar}, nil
4038
}

‎coder-sdk/devurl.go

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import (
66
"net/http"
77
)
88

9-
// DevURL is the parsed json response record for a devURL from cemanager
9+
// DevURL is the parsed json response record for a devURL from cemanager.
1010
type DevURL struct {
1111
ID string `json:"id" tab:"ID"`
1212
URL string `json:"url" tab:"URL"`
@@ -20,21 +20,21 @@ type delDevURLRequest struct {
2020
DevURLID string `json:"url_id"`
2121
}
2222

23-
// DelDevURL deletes the specified devurl
23+
// DelDevURL deletes the specified devurl.
2424
func (c Client) DelDevURL(ctx context.Context, envID, urlID string) error {
2525
reqURL := fmt.Sprintf("/api/environments/%s/devurls/%s", envID, urlID)
2626

27-
res, err := c.request(ctx, http.MethodDelete, reqURL, delDevURLRequest{
27+
resp, err := c.request(ctx, http.MethodDelete, reqURL, delDevURLRequest{
2828
EnvID: envID,
2929
DevURLID: urlID,
3030
})
3131
if err != nil {
3232
return err
3333
}
34-
defer res.Body.Close()
34+
defer func() { _ = resp.Body.Close() }() // Best effort. Likefly connection drop.
3535

36-
if res.StatusCode != http.StatusOK {
37-
return bodyError(res)
36+
if resp.StatusCode != http.StatusOK {
37+
return bodyError(resp)
3838
}
3939

4040
return nil
@@ -47,11 +47,11 @@ type createDevURLRequest struct {
4747
Name string `json:"name"`
4848
}
4949

50-
// InsertDevURL inserts a new devurl for the authenticated user
50+
// InsertDevURL inserts a new devurl for the authenticated user.
5151
func (c Client) InsertDevURL(ctx context.Context, envID string, port int, name, access string) error {
5252
reqURL := fmt.Sprintf("/api/environments/%s/devurls", envID)
5353

54-
res, err := c.request(ctx, http.MethodPost, reqURL, createDevURLRequest{
54+
resp, err := c.request(ctx, http.MethodPost, reqURL, createDevURLRequest{
5555
EnvID: envID,
5656
Port: port,
5757
Access: access,
@@ -60,22 +60,22 @@ func (c Client) InsertDevURL(ctx context.Context, envID string, port int, name,
6060
if err != nil {
6161
return err
6262
}
63-
defer res.Body.Close()
63+
defer func() { _ = resp.Body.Close() }() // Best effort. Likefly connection drop.
6464

65-
if res.StatusCode != http.StatusOK {
66-
return bodyError(res)
65+
if resp.StatusCode != http.StatusOK {
66+
return bodyError(resp)
6767
}
6868

6969
return nil
7070
}
7171

7272
type updateDevURLRequest createDevURLRequest
7373

74-
// UpdateDevURL updates an existing devurl for the authenticated user
74+
// UpdateDevURL updates an existing devurl for the authenticated user.
7575
func (c Client) UpdateDevURL(ctx context.Context, envID, urlID string, port int, name, access string) error {
7676
reqURL := fmt.Sprintf("/api/environments/%s/devurls/%s", envID, urlID)
7777

78-
res, err := c.request(ctx, http.MethodPut, reqURL, updateDevURLRequest{
78+
resp, err := c.request(ctx, http.MethodPut, reqURL, updateDevURLRequest{
7979
EnvID: envID,
8080
Port: port,
8181
Access: access,
@@ -84,10 +84,10 @@ func (c Client) UpdateDevURL(ctx context.Context, envID, urlID string, port int,
8484
if err != nil {
8585
return err
8686
}
87-
defer res.Body.Close()
87+
defer func() { _ = resp.Body.Close() }() // Best effort. Likefly connection drop.
8888

89-
if res.StatusCode != http.StatusOK {
90-
return bodyError(res)
89+
if resp.StatusCode != http.StatusOK {
90+
return bodyError(resp)
9191
}
9292

9393
return nil

‎coder-sdk/env.go

Lines changed: 26 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -29,13 +29,18 @@ type Environment struct {
2929
UpdatedAt time.Time `json:"updated_at" tab:"-"`
3030
LastOpenedAt time.Time `json:"last_opened_at" tab:"-"`
3131
LastConnectionAt time.Time `json:"last_connection_at" tab:"-"`
32-
AutoOffThreshold xjson.Duration `json:"auto_off_threshold" tab:"-"`
32+
AutoOffThreshold xjson.MSDuration `json:"auto_off_threshold" tab:"-"`
3333
}
3434

3535
// RebuildMessage defines the message shown when an Environment requires a rebuild for it can be accessed.
3636
type RebuildMessage struct {
37-
Text string `json:"text"`
38-
Required bool `json:"required"`
37+
Text string `json:"text"`
38+
Required bool `json:"required"`
39+
AutoOffThreshold xjson.MSDuration `json:"auto_off_threshold" tab:"-"`
40+
RebuildMessages []struct {
41+
Text string `json:"text"`
42+
Required bool `json:"required"`
43+
} `json:"rebuild_messages" tab:"-"`
3944
}
4045

4146
// EnvironmentStat represents the state of an environment
@@ -51,9 +56,7 @@ type EnvironmentStat struct {
5156
DiskUsed int64 `json:"disk_used"`
5257
}
5358

54-
func (e EnvironmentStat) String() string {
55-
return string(e.ContainerStatus)
56-
}
59+
func (e EnvironmentStat) String() string { return string(e.ContainerStatus) }
5760

5861
// EnvironmentStatus refers to the states of an environment.
5962
type EnvironmentStatus string
@@ -67,7 +70,7 @@ const (
6770
EnvironmentUnknown EnvironmentStatus = "UNKNOWN"
6871
)
6972

70-
// CreateEnvironmentRequest is used to configure a new environment
73+
// CreateEnvironmentRequest is used to configure a new environment.
7174
type CreateEnvironmentRequest struct {
7275
Name string `json:"name"`
7376
ImageID string `json:"image_id"`
@@ -81,50 +84,39 @@ type CreateEnvironmentRequest struct {
8184

8285
// CreateEnvironment sends a request to create an environment.
8386
func (c Client) CreateEnvironment(ctx context.Context, orgID string, req CreateEnvironmentRequest) (*Environment, error) {
84-
var env *Environment
85-
err := c.requestBody(
86-
ctx,
87-
http.MethodPost, "/api/orgs/"+orgID+"/environments",
88-
req,
89-
env,
90-
)
91-
return env, err
87+
var env Environment
88+
if err := c.requestBody(ctx, http.MethodPost, "/api/orgs/"+orgID+"/environments", req, &env); err != nil {
89+
return nil, err
90+
}
91+
return &env, nil
9292
}
9393

9494
// EnvironmentsByOrganization gets the list of environments owned by the given user.
9595
func (c Client) EnvironmentsByOrganization(ctx context.Context, userID, orgID string) ([]Environment, error) {
9696
var envs []Environment
97-
err := c.requestBody(
98-
ctx,
99-
http.MethodGet, "/api/orgs/"+orgID+"/members/"+userID+"/environments",
100-
nil,
101-
&envs,
102-
)
103-
return envs, err
97+
if err := c.requestBody(ctx, http.MethodGet, "/api/orgs/"+orgID+"/members/"+userID+"/environments", nil, &envs); err != nil {
98+
return nil, err
99+
}
100+
return envs, nil
104101
}
105102

106103
// DeleteEnvironment deletes the environment.
107104
func (c Client) DeleteEnvironment(ctx context.Context, envID string) error {
108-
return c.requestBody(
109-
ctx,
110-
http.MethodDelete, "/api/environments/"+envID,
111-
nil,
112-
nil,
113-
)
105+
return c.requestBody(ctx, http.MethodDelete, "/api/environments/"+envID, nil, nil)
114106
}
115107

116108
// DialWsep dials an environments command execution interface
117-
// See github.com/cdr/wsep for details
109+
// See https://github.com/cdr/wsep for details.
118110
func (c Client) DialWsep(ctx context.Context, env *Environment) (*websocket.Conn, error) {
119-
return c.dialWs(ctx, "/proxy/environments/"+env.ID+"/wsep")
111+
return c.dialWebsocket(ctx, "/proxy/environments/"+env.ID+"/wsep")
120112
}
121113

122-
// DialEnvironmentBuildLog opens a websocket connection for the environment build log messages
114+
// DialEnvironmentBuildLog opens a websocket connection for the environment build log messages.
123115
func (c Client) DialEnvironmentBuildLog(ctx context.Context, envID string) (*websocket.Conn, error) {
124-
return c.dialWs(ctx, "/api/environments/"+envID+"/watch-update")
116+
return c.dialWebsocket(ctx, "/api/environments/"+envID+"/watch-update")
125117
}
126118

127-
// DialEnvironmentStats opens a websocket connection for environment stats
119+
// DialEnvironmentStats opens a websocket connection for environment stats.
128120
func (c Client) DialEnvironmentStats(ctx context.Context, envID string) (*websocket.Conn, error) {
129-
return c.dialWs(ctx, "/api/environments/"+envID+"/watch-stats")
121+
return c.dialWebsocket(ctx, "/api/environments/"+envID+"/watch-stats")
130122
}

‎coder-sdk/error.go

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,18 +2,20 @@ package coder
22

33
import (
44
"encoding/json"
5+
"errors"
56
"net/http"
67
"net/http/httputil"
78

89
"golang.org/x/xerrors"
910
)
1011

11-
// ErrNotFound describes an error case in which the requested resource could not be found
12-
var ErrNotFound = xerrors.Errorf("resource not found")
12+
// ErrNotFound describes an error case in which the requested resource could not be found.
13+
var ErrNotFound = errors.New("resource not found")
1314

15+
// apiError is the expected payload format for our errors.
1416
type apiError struct {
1517
Err struct {
16-
Msg string `json:"msg,required"`
18+
Msg string `json:"msg"` // Required.
1719
} `json:"error"`
1820
}
1921

@@ -24,9 +26,12 @@ func bodyError(resp *http.Response) error {
2426
}
2527

2628
var msg apiError
27-
err = json.NewDecoder(resp.Body).Decode(&msg)
28-
if err != nil || msg.Err.Msg == "" {
29+
// Try to decode the payload as an error, if it fails or if there is no error message,
30+
// return the response URL with the dump.
31+
if err = json.NewDecoder(resp.Body).Decode(&msg); err != nil || msg.Err.Msg == "" {
2932
return xerrors.Errorf("%s\n%s", resp.Request.URL, byt)
3033
}
34+
35+
// If the payload was a in the expected error format with a message, include it.
3136
return xerrors.Errorf("%s\n%s%s", resp.Request.URL, byt, msg.Err.Msg)
3237
}

‎coder-sdk/image.go

Lines changed: 15 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ type Image struct {
1111
OrganizationID string `json:"organization_id"`
1212
Repository string `json:"repository"`
1313
Description string `json:"description"`
14-
URL string `json:"url"` // user-supplied URL for image
14+
URL string `json:"url"` // User-supplied URL for image.
1515
DefaultCPUCores float32 `json:"default_cpu_cores"`
1616
DefaultMemoryGB int `json:"default_memory_gb"`
1717
DefaultDiskGB int `json:"default_disk_gb"`
@@ -28,28 +28,22 @@ type NewRegistryRequest struct {
2828

2929
// ImportImageRequest is used to import new images and registries into Coder
3030
type ImportImageRequest struct {
31-
// RegistryID is used to import images to existing registries.
32-
RegistryID *string `json:"registry_id"`
33-
// NewRegistry is used when adding a new registry.
34-
NewRegistry *NewRegistryRequest `json:"new_registry"`
35-
// Repository refers to the image. For example: "codercom/ubuntu".
36-
Repository string `json:"repository"`
37-
Tag string `json:"tag"`
38-
DefaultCPUCores float32 `json:"default_cpu_cores"`
39-
DefaultMemoryGB int `json:"default_memory_gb"`
40-
DefaultDiskGB int `json:"default_disk_gb"`
41-
Description string `json:"description"`
42-
URL string `json:"url"`
31+
RegistryID *string `json:"registry_id"` // Used to import images to existing registries.
32+
NewRegistry *NewRegistryRequest `json:"new_registry"` // Used when adding a new registry.
33+
Repository string `json:"repository"` // Refers to the image. Ex: "codercom/ubuntu".
34+
Tag string `json:"tag"`
35+
DefaultCPUCores float32 `json:"default_cpu_cores"`
36+
DefaultMemoryGB int `json:"default_memory_gb"`
37+
DefaultDiskGB int `json:"default_disk_gb"`
38+
Description string `json:"description"`
39+
URL string `json:"url"`
4340
}
4441

4542
// ImportImage creates a new image and optionally a new registry
4643
func (c Client) ImportImage(ctx context.Context, orgID string, req ImportImageRequest) (*Image, error) {
47-
var img *Image
48-
err := c.requestBody(
49-
ctx,
50-
http.MethodPost, "/api/orgs/"+orgID+"/images",
51-
req,
52-
img,
53-
)
54-
return img, err
44+
var img Image
45+
if err := c.requestBody(ctx, http.MethodPost, "/api/orgs/"+orgID+"/images", req, &img); err != nil {
46+
return nil, err
47+
}
48+
return &img, nil
5549
}

0 commit comments

Comments
 (0)