Skip to content

Commit

Permalink
Update all Update methods to return the unmarshalled struct, rather…
Browse files Browse the repository at this point in the history 
… than unmarshalling into the input

encoding/json behaves surprisingly ([#27172](golang/go#27172), [#31924](golang/go#31924), [#26946](golang/go#26946), [#21092](golang/go#21092)) when unmarshalling into non-empty structs, and this behaviour is unexpected in this library anyway; no other methods do any user-facing unmarshalling or pointer mutation.

This commit changes all of the `Update` methods that unmarshall into their inputs to no longer do that, and return a newly-allocated struct instead.
  • Loading branch information
@moskyb
moskyb committed Oct 16, 2024
1 parent 47e1b20 commit ce355c1
Show file tree
Hide file tree
Showing 13 changed files with 108 additions and 264 deletions.
25 changes: 16 additions & 9 deletions buildkite/cluster_queues.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ type ClusterQueue struct {
DispatchPaused bool `json:"dispatch_paused,omitempty" yaml:"dispatch_paused,omitempty"`
DispatchPausedBy *ClusterCreator `json:"dispatch_paused_by,omitempty" yaml:"dispatch_paused_by,omitempty"`
DispatchPausedAt *Timestamp `json:"dispatch_paused_at,omitempty" yaml:"dispatch_paused_at,omitempty"`
DispatchPausedNote *string `json:"dispatch_paused_note,omitempty" yaml:"dispatch_paused_note,omitempty"`
DispatchPausedNote string `json:"dispatch_paused_note,omitempty" yaml:"dispatch_paused_note,omitempty"`
CreatedAt *Timestamp `json:"created_at,omitempty" yaml:"created_at,omitempty"`
CreatedBy ClusterCreator `json:"created_by,omitempty" yaml:"created_by,omitempty"`
}
Expand Down Expand Up @@ -100,19 +100,20 @@ func (cqs *ClusterQueuesService) Create(ctx context.Context, org, clusterID stri
return queue, resp, err
}

func (cqs *ClusterQueuesService) Update(ctx context.Context, org, clusterID, queueID string, qu ClusterQueueUpdate) (*Response, error) {
func (cqs *ClusterQueuesService) Update(ctx context.Context, org, clusterID, queueID string, qu ClusterQueueUpdate) (ClusterQueue, *Response, error) {
u := fmt.Sprintf("v2/organizations/%s/clusters/%s/queues/%s", org, clusterID, queueID)
req, err := cqs.client.NewRequest(ctx, "PATCH", u, qu)
if err != nil {
return nil, err
return ClusterQueue{}, nil, err
}

resp, err := cqs.client.Do(req, nil)
var cq ClusterQueue
resp, err := cqs.client.Do(req, &cq)
if err != nil {
return resp, err
return ClusterQueue{}, resp, err
}

return resp, err
return cq, resp, err
}

func (cqs *ClusterQueuesService) Delete(ctx context.Context, org, clusterID, queueID string) (*Response, error) {
Expand All @@ -125,14 +126,20 @@ func (cqs *ClusterQueuesService) Delete(ctx context.Context, org, clusterID, que
return cqs.client.Do(req, nil)
}

func (cqs *ClusterQueuesService) Pause(ctx context.Context, org, clusterID, queueID string, qp ClusterQueuePause) (*Response, error) {
func (cqs *ClusterQueuesService) Pause(ctx context.Context, org, clusterID, queueID string, qp ClusterQueuePause) (ClusterQueue, *Response, error) {
u := fmt.Sprintf("v2/organizations/%s/clusters/%s/queues/%s/pause_dispatch", org, clusterID, queueID)
req, err := cqs.client.NewRequest(ctx, "POST", u, qp)
if err != nil {
return nil, err
return ClusterQueue{}, nil, err
}

return cqs.client.Do(req, nil)
var cq ClusterQueue
resp, err := cqs.client.Do(req, &cq)
if err != nil {
return ClusterQueue{}, resp, err
}

return cq, resp, err
}

func (cqs *ClusterQueuesService) Resume(ctx context.Context, org, clusterID, queueID string) (*Response, error) {
Expand Down
111 changes: 23 additions & 88 deletions buildkite/cluster_queues_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ func TestClusterQueuesService_List(t *testing.T) {
DispatchPaused: true,
DispatchPausedBy: &clusterCreator,
DispatchPausedAt: NewTimestamp(devQueuePausedAt),
DispatchPausedNote: String("Weekend queue pause"),
DispatchPausedNote: "Weekend queue pause",
CreatedAt: NewTimestamp(devQueueClusterCreatedAt),
CreatedBy: clusterCreator,
},
Expand Down Expand Up @@ -199,7 +199,7 @@ func TestClusterQueuesService_Get(t *testing.T) {
DispatchPaused: true,
DispatchPausedBy: &clusterCreator,
DispatchPausedAt: NewTimestamp(devQueuePausedAt),
DispatchPausedNote: String("Weekend queue pause"),
DispatchPausedNote: "Weekend queue pause",
CreatedAt: NewTimestamp(devQueueClusterCreatedAt),
CreatedBy: clusterCreator,
}
Expand Down Expand Up @@ -260,39 +260,6 @@ func TestClusterQueuesService_Update(t *testing.T) {
server, client, teardown := newMockServerAndClient(t)
t.Cleanup(teardown)

input := ClusterQueueCreate{
Key: "development1",
Description: "Development 1 queue",
}

server.HandleFunc("/v2/organizations/my-great-org/clusters/b7c9bc4f-526f-4c18-a3be-dc854ab75d57/queues", func(w http.ResponseWriter, r *http.Request) {
var v ClusterQueueCreate
json.NewDecoder(r.Body).Decode(&v)

testMethod(t, r, "POST")

if diff := cmp.Diff(v, input); diff != "" {
t.Errorf("Request body diff: (-got +want)\n%s", diff)
}

fmt.Fprint(w,
`
{
"id" : "1374ffd0-c5ed-49a5-aebe-67ce906e68ca",
"key" : "development1",
"description": "Development 1 queue"
}`)
})

queue, _, err := client.ClusterQueues.Create(context.Background(), "my-great-org", "b7c9bc4f-526f-4c18-a3be-dc854ab75d57", input)

if err != nil {
t.Errorf("TestClusterQueues.Update returned error: %v", err)
}

// Lets update the description of the cluster queue
queue.Description = "Development 1 Team queue"

server.HandleFunc("/v2/organizations/my-great-org/clusters/b7c9bc4f-526f-4c18-a3be-dc854ab75d57/queues/1374ffd0-c5ed-49a5-aebe-67ce906e68ca", func(w http.ResponseWriter, r *http.Request) {
v := new(ClusterQueueUpdate)
json.NewDecoder(r.Body).Decode(&v)
Expand All @@ -308,11 +275,9 @@ func TestClusterQueuesService_Update(t *testing.T) {
}`)
})

queueUpdate := ClusterQueueUpdate{
Description: "Development 1 Team queue",
}
queueUpdate := ClusterQueueUpdate{Description: "Development 1 Team queue"}

_, err = client.ClusterQueues.Update(context.Background(), "my-great-org", "b7c9bc4f-526f-4c18-a3be-dc854ab75d57", "1374ffd0-c5ed-49a5-aebe-67ce906e68ca", queueUpdate)
got, _, err := client.ClusterQueues.Update(context.Background(), "my-great-org", "b7c9bc4f-526f-4c18-a3be-dc854ab75d57", "1374ffd0-c5ed-49a5-aebe-67ce906e68ca", queueUpdate)

if err != nil {
t.Errorf("TestClusterQueues.Update returned error: %v", err)
Expand All @@ -324,7 +289,7 @@ func TestClusterQueuesService_Update(t *testing.T) {
Description: "Development 1 Team queue",
}

if diff := cmp.Diff(queue, want); diff != "" {
if diff := cmp.Diff(got, want); diff != "" {
t.Errorf("TestClusterQueues.Update diff: (-got +want)\n%s", diff)
}
}
Expand Down Expand Up @@ -352,73 +317,43 @@ func TestClusterQueuesService_Pause(t *testing.T) {
server, client, teardown := newMockServerAndClient(t)
t.Cleanup(teardown)

input := ClusterQueueCreate{
Key: "development1",
Description: "Development 1 Team queue",
}
clusterUUID := "b7c9bc4f-526f-4c18-a3be-dc854ab75d57"
queueUUID := "b7c9bc4f-526f-4c18-a3be-dc854ab75d57"

server.HandleFunc("/v2/organizations/my-great-org/clusters/b7c9bc4f-526f-4c18-a3be-dc854ab75d57/queues", func(w http.ResponseWriter, r *http.Request) {
var v ClusterQueueCreate
json.NewDecoder(r.Body).Decode(&v)

testMethod(t, r, "POST")

if diff := cmp.Diff(v, input); diff != "" {
t.Errorf("Request body diff: (-got +want)\n%s", diff)
pauseEndpoint := fmt.Sprintf("/v2/organizations/my-great-org/clusters/%s/queues/%s/pause_dispatch", clusterUUID, queueUUID)
server.HandleFunc(pauseEndpoint, func(w http.ResponseWriter, r *http.Request) {
var v ClusterQueuePause
err := json.NewDecoder(r.Body).Decode(&v)
if err != nil {
t.Errorf("TestClusterQueues.Pause error decoding request: %v", err)
}

fmt.Fprint(w,
`
{
"id" : "5cadac07-51dd-4e12-bea3-d91be4655c2f",
"key" : "development1",
"description": "Development 1 Team queue"
}`)
})

queue, _, err := client.ClusterQueues.Create(context.Background(), "my-great-org", "b7c9bc4f-526f-4c18-a3be-dc854ab75d57", input)

if err != nil {
t.Errorf("TestClusterQueues.Pause returned error: %v", err)
}

// Update the dispatch paused note of the queue
queue.DispatchPausedNote = String("Pausing dispatch for the weekend")

server.HandleFunc("/v2/organizations/my-great-org/clusters/b7c9bc4f-526f-4c18-a3be-dc854ab75d57/queues/5cadac07-51dd-4e12-bea3-d91be4655c2f/pause_dispatch", func(w http.ResponseWriter, r *http.Request) {
v := new(ClusterQueueUpdate)
json.NewDecoder(r.Body).Decode(&v)

testMethod(t, r, "POST")

fmt.Fprint(w,
fmt.Fprintf(w,
`
{
"id" : "5cadac07-51dd-4e12-bea3-d91be4655c2f",
"id" : %q,
"key" : "development1",
"description": "Development 1 Team queue"
"dispatch_paused_note": "Pausing dispatch for the weekend"",
}`)
"description": "Development 1 Team queue",
"dispatch_paused_note": "Pausing dispatch for the weekend"
}`, queueUUID)
})

queuePause := ClusterQueuePause{
Note: "Pausing dispatch for the weekend",
}

_, err = client.ClusterQueues.Pause(context.Background(), "my-great-org", "b7c9bc4f-526f-4c18-a3be-dc854ab75d57", "5cadac07-51dd-4e12-bea3-d91be4655c2f", queuePause)

queuePause := ClusterQueuePause{Note: "Pausing dispatch for the weekend"}
got, _, err := client.ClusterQueues.Pause(context.Background(), "my-great-org", clusterUUID, queueUUID, queuePause)
if err != nil {
t.Errorf("TestClusterQueues.Pause returned error: %v", err)
}

want := ClusterQueue{
ID: "5cadac07-51dd-4e12-bea3-d91be4655c2f",
ID: queueUUID,
Key: "development1",
Description: "Development 1 Team queue",
DispatchPausedNote: String("Pausing dispatch for the weekend"),
DispatchPausedNote: "Pausing dispatch for the weekend",
}

if diff := cmp.Diff(queue, want); diff != "" {
if diff := cmp.Diff(got, want); diff != "" {
t.Errorf("TestClusterQueues.Pause diff: (-got +want)\n%s", diff)
}
}
Expand Down
11 changes: 6 additions & 5 deletions buildkite/cluster_tokens.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,19 +87,20 @@ func (cts *ClusterTokensService) Create(ctx context.Context, org, clusterID stri
return token, resp, err
}

func (cts *ClusterTokensService) Update(ctx context.Context, org, clusterID, tokenID string, ctc ClusterTokenCreateUpdate) (*Response, error) {
func (cts *ClusterTokensService) Update(ctx context.Context, org, clusterID, tokenID string, ctc ClusterTokenCreateUpdate) (ClusterToken, *Response, error) {
u := fmt.Sprintf("v2/organizations/%s/clusters/%s/tokens/%s", org, clusterID, tokenID)
req, err := cts.client.NewRequest(ctx, "PATCH", u, ctc)
if err != nil {
return nil, err
return ClusterToken{}, nil, err
}

resp, err := cts.client.Do(req, nil)
var ct ClusterToken
resp, err := cts.client.Do(req, &ct)
if err != nil {
return resp, err
return ClusterToken{}, resp, err
}

return resp, err
return ct, resp, err
}

func (cts *ClusterTokensService) Delete(ctx context.Context, org, clusterID, tokenID string) (*Response, error) {
Expand Down
32 changes: 2 additions & 30 deletions buildkite/cluster_tokens_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,34 +209,6 @@ func TestClusterTokensService_Update(t *testing.T) {
server, client, teardown := newMockServerAndClient(t)
t.Cleanup(teardown)

input := ClusterTokenCreateUpdate{Description: "Development 1 Fleet Token"}

server.HandleFunc("/v2/organizations/my-great-org/clusters/b7c9bc4f-526f-4c18-a3be-dc854ab75d57/tokens", func(w http.ResponseWriter, r *http.Request) {
var v ClusterTokenCreateUpdate
json.NewDecoder(r.Body).Decode(&v)

testMethod(t, r, "POST")

if diff := cmp.Diff(v, input); diff != "" {
t.Errorf("Request body diff: (-got +want)\n%s", diff)
}

fmt.Fprint(w,
`
{
"id": "9cb33339-1c4a-4020-9aeb-3319b2e1f054",
"description": "Development 1 agent-fleet token"
}`)
})

token, _, err := client.ClusterTokens.Create(context.Background(), "my-great-org", "b7c9bc4f-526f-4c18-a3be-dc854ab75d57", input)
if err != nil {
t.Errorf("TestClusterTokens.Update returned error: %v", err)
}

// Lets update the description of the cluster token
token.Description = "Development 1 agent token"

server.HandleFunc("/v2/organizations/my-great-org/clusters/b7c9bc4f-526f-4c18-a3be-dc854ab75d57/tokens/9cb33339-1c4a-4020-9aeb-3319b2e1f054", func(w http.ResponseWriter, r *http.Request) {
v := new(ClusterTokenCreateUpdate)
json.NewDecoder(r.Body).Decode(&v)
Expand All @@ -253,7 +225,7 @@ func TestClusterTokensService_Update(t *testing.T) {

tokenUpdate := ClusterTokenCreateUpdate{Description: "Development 1 agent token"}

_, err = client.ClusterTokens.Update(context.Background(), "my-great-org", "b7c9bc4f-526f-4c18-a3be-dc854ab75d57", "9cb33339-1c4a-4020-9aeb-3319b2e1f054", tokenUpdate)
got, _, err := client.ClusterTokens.Update(context.Background(), "my-great-org", "b7c9bc4f-526f-4c18-a3be-dc854ab75d57", "9cb33339-1c4a-4020-9aeb-3319b2e1f054", tokenUpdate)
if err != nil {
t.Errorf("TestClusterTokens.Update returned error: %v", err)
}
Expand All @@ -263,7 +235,7 @@ func TestClusterTokensService_Update(t *testing.T) {
Description: "Development 1 agent token",
}

if diff := cmp.Diff(token, want); diff != "" {
if diff := cmp.Diff(got, want); diff != "" {
t.Errorf("TestClusterTokens.Update diff: (-got +want)\n%s", diff)
}
}
Expand Down
11 changes: 6 additions & 5 deletions buildkite/clusters.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,19 +109,20 @@ func (cs *ClustersService) Create(ctx context.Context, org string, cc ClusterCre
return cluster, resp, err
}

func (cs *ClustersService) Update(ctx context.Context, org, id string, cu ClusterUpdate) (*Response, error) {
func (cs *ClustersService) Update(ctx context.Context, org, id string, cu ClusterUpdate) (Cluster, *Response, error) {
u := fmt.Sprintf("v2/organizations/%s/clusters/%s", org, id)
req, err := cs.client.NewRequest(ctx, "PATCH", u, cu)
if err != nil {
return nil, nil
return Cluster{}, nil, nil
}

resp, err := cs.client.Do(req, nil)
var cluster Cluster
resp, err := cs.client.Do(req, &cluster)
if err != nil {
return resp, err
return Cluster{}, resp, err
}

return resp, err
return cluster, resp, err
}

func (cs *ClustersService) Delete(ctx context.Context, org, id string) (*Response, error) {
Expand Down
42 changes: 5 additions & 37 deletions buildkite/clusters_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -243,43 +243,11 @@ func TestClustersService_Update(t *testing.T) {
server, client, teardown := newMockServerAndClient(t)
t.Cleanup(teardown)

input := ClusterCreate{
Name: "Testing Cluster",
Description: "A cluster for testing",
Emoji: ":construction:",
Color: "E5F185",
}

server.HandleFunc("/v2/organizations/my-great-org/clusters", func(w http.ResponseWriter, r *http.Request) {
var v ClusterCreate
json.NewDecoder(r.Body).Decode(&v)

testMethod(t, r, "POST")

if diff := cmp.Diff(v, input); diff != "" {
t.Errorf("$3 diff: (-got +want)\n%s", diff)
}

fmt.Fprint(w,
`
{
"id": "a32cbe81-82b2-45f7-bd97-66f1ac2c0cc1",
"name" : "Testing Cluster",
"description": "A cluster for testing",
"emoji": ":construction:",
"color": "E5F185"
}`)
})

cluster, _, err := client.Clusters.Create(context.Background(), "my-great-org", input)
if err != nil {
t.Errorf("TestClusters.Create returned error: %v", err)
}

// Lets update the description of the cluster
cluster.Description = "A test cluster"
orgSlug := "my-great-org"
clusterID := "a32cbe81-82b2-45f7-bd97-66f1ac2c0cc1"

server.HandleFunc("/v2/organizations/my-great-org/clusters/a32cbe81-82b2-45f7-bd97-66f1ac2c0cc1", func(w http.ResponseWriter, r *http.Request) {
clustersPutEndpoint := fmt.Sprintf("/v2/organizations/%s/clusters/%s", orgSlug, clusterID)
server.HandleFunc(clustersPutEndpoint, func(w http.ResponseWriter, r *http.Request) {
var v ClusterUpdate
json.NewDecoder(r.Body).Decode(&v)

Expand All @@ -297,7 +265,7 @@ func TestClustersService_Update(t *testing.T) {
})

clusterUpdate := ClusterUpdate{Description: "A test cluster"}
_, err = client.Clusters.Update(context.Background(), "my-great-org", cluster.ID, clusterUpdate)
cluster, _, err := client.Clusters.Update(context.Background(), orgSlug, clusterID, clusterUpdate)
if err != nil {
t.Errorf("TestClusters.Update returned error: %v", err)
}
Expand Down
Loading

0 comments on commit ce355c1

Please sign in to comment.