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 2eba4f0 commit a7e65d3
Show file tree
Hide file tree
Showing 12 changed files with 73 additions and 176 deletions.
11 changes: 6 additions & 5 deletions buildkite/cluster_queues.go
View file
Original file line number Diff line number Diff line change
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 Down
13 changes: 2 additions & 11 deletions buildkite/cluster_queues_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -284,15 +284,6 @@ func TestClusterQueuesService_Update(t *testing.T) {
}`)
})

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 @@ -312,7 +303,7 @@ func TestClusterQueuesService_Update(t *testing.T) {
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 +315,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
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
12 changes: 9 additions & 3 deletions buildkite/pipeline_templates.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,14 +104,20 @@ func (pts *PipelineTemplatesService) Create(ctx context.Context, org string, ptc
return template, resp, err
}

func (pts *PipelineTemplatesService) Update(ctx context.Context, org, templateUUID string, ptu PipelineTemplateCreateUpdate) (*Response, error) {
func (pts *PipelineTemplatesService) Update(ctx context.Context, org, templateUUID string, ptu PipelineTemplateCreateUpdate) (PipelineTemplate, *Response, error) {
u := fmt.Sprintf("v2/organizations/%s/pipeline-templates/%s", org, templateUUID)
req, err := pts.client.NewRequest(ctx, "PATCH", u, ptu)
if err != nil {
return nil, err
return PipelineTemplate{}, nil, err
}

return pts.client.Do(req, nil)
var template PipelineTemplate
resp, err := pts.client.Do(req, &template)
if err != nil {
return PipelineTemplate{}, resp, err
}

return template, resp, err
}

func (pts *PipelineTemplatesService) Delete(ctx context.Context, org, templateUUID string) (*Response, error) {
Expand Down
45 changes: 3 additions & 42 deletions buildkite/pipeline_templates_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -275,44 +275,6 @@ func TestPipelineTemplatesService_Update(t *testing.T) {
server, client, teardown := newMockServerAndClient(t)
t.Cleanup(teardown)

input := PipelineTemplateCreateUpdate{
Name: "Production Pipeline uploader",
Description: "Production pipeline upload template",
Configuration: "steps:\n - label: \":pipeline:\"\n command: \"buildkite-agent pipeline upload .buildkite/pipeline-production.yml\"\n",
Available: true,
}

server.HandleFunc("/v2/organizations/my-great-org/pipeline-templates", func(w http.ResponseWriter, r *http.Request) {
var v PipelineTemplateCreateUpdate
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,
`
{
"uuid": "b8c2e171-1c7d-47a4-a4d1-a20d691f51d0",
"graphql_id": "UGlwZWxpbmVUZW1wbG5lLS0tYjhjMmUxNzEtMWM3ZC00N2E0LWE0ZDEtYTIwZDY5MWY1MWQw==",
"name" : "Production Pipeline template",
"description": "Production pipeline upload template",
"configuration": "steps:\n - label: \":pipeline:\"\n command: \"buildkite-agent pipeline upload .buildkite/pipeline-production.yml\"\n",
"available": true
}`)
})

pipelineTemplate, _, err := client.PipelineTemplates.Create(context.Background(), "my-great-org", input)

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

// Lets update the description of the pipeline template
pipelineTemplate.Description = "A pipeline template for uploading a production pipeline YAML (pipeline-production.yml"

server.HandleFunc("/v2/organizations/my-great-org/pipeline-templates/b8c2e171-1c7d-47a4-a4d1-a20d691f51d0", func(w http.ResponseWriter, r *http.Request) {
var v PipelineTemplateCreateUpdate
json.NewDecoder(r.Body).Decode(&v)
Expand All @@ -335,8 +297,7 @@ func TestPipelineTemplatesService_Update(t *testing.T) {
Description: "A pipeline template for uploading a production pipeline YAML (pipeline-production.yml",
}

_, err = client.PipelineTemplates.Update(context.Background(), "my-great-org", "b8c2e171-1c7d-47a4-a4d1-a20d691f51d0", pipelineTemplateUpdate)

got, _, err := client.PipelineTemplates.Update(context.Background(), "my-great-org", "b8c2e171-1c7d-47a4-a4d1-a20d691f51d0", pipelineTemplateUpdate)
if err != nil {
t.Errorf("TestPipelineTemplates.Update returned error: %v", err)
}
Expand All @@ -345,12 +306,12 @@ func TestPipelineTemplatesService_Update(t *testing.T) {
UUID: "b8c2e171-1c7d-47a4-a4d1-a20d691f51d0",
GraphQLID: "UGlwZWxpbmVUZW1wbG5lLS0tYjhjMmUxNzEtMWM3ZC00N2E0LWE0ZDEtYTIwZDY5MWY1MWQw==",
Name: "Production Pipeline template",
Description: "A pipeline template for uploading a production pipeline YAML (pipeline-production.yml",
Description: "A pipeline template for uploading a production pipeline YAML (pipeline-production.yml)",
Configuration: "steps:\n - label: \":pipeline:\"\n command: \"buildkite-agent pipeline upload .buildkite/pipeline-production.yml\"\n",
Available: true,
}

if diff := cmp.Diff(pipelineTemplate, want); diff != "" {
if diff := cmp.Diff(got, want); diff != "" {
t.Errorf("TestPipelineTemplates.Update diff: (-got +want)\n%s", diff)
}
}
Expand Down
20 changes: 9 additions & 11 deletions examples/cluster_queues/update/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,12 @@ import (
)

var (
apiToken = kingpin.Flag("token", "API token").Required().String()
org = kingpin.Flag("org", "Orginization slug").Required().String()
clusterID = kingpin.Flag("clusterID", "Cluster UUID").Required().String()
queueID = kingpin.Flag("queueID", "Cluster queue UUID").Required().String()
debug = kingpin.Flag("debug", "Enable debugging").Bool()
apiToken = kingpin.Flag("token", "API token").Required().String()
org = kingpin.Flag("org", "Orginization slug").Required().String()
clusterID = kingpin.Flag("clusterID", "Cluster UUID").Required().String()
queueID = kingpin.Flag("queueID", "Cluster queue UUID").Required().String()
newDescription = kingpin.Flag("description", "New description for the cluster queue").Required().String()
debug = kingpin.Flag("debug", "Enable debugging").Bool()
)

func main() {
Expand All @@ -25,15 +26,12 @@ func main() {
log.Fatalf("creating buildkite API client failed: %v", err)
}

clusterQueueUpdate := buildkite.ClusterQueueUpdate{
Description: "Development team cluster queue",
}

resp, err := client.ClusterQueues.Update(context.Background(), *org, *clusterID, *queueID, clusterQueueUpdate)
clusterQueueUpdate := buildkite.ClusterQueueUpdate{Description: *newDescription}

cq, _, err := client.ClusterQueues.Update(context.Background(), *org, *clusterID, *queueID, clusterQueueUpdate)
if err != nil {
log.Fatalf("Updating cluster queue failed: %s", err)
}

fmt.Println(resp.StatusCode)
fmt.Printf("Updated cluster queue: %v\n", cq)
}
17 changes: 9 additions & 8 deletions examples/cluster_tokens/update/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,12 @@ import (
)

var (
apiToken = kingpin.Flag("token", "API token").Required().String()
org = kingpin.Flag("org", "Orginization slug").Required().String()
clusterID = kingpin.Flag("clusterID", "Cluster UUID").Required().String()
tokenID = kingpin.Flag("tokenID", "Cluster token UUID").Required().String()
debug = kingpin.Flag("debug", "Enable debugging").Bool()
apiToken = kingpin.Flag("token", "API token").Required().String()
org = kingpin.Flag("org", "Orginization slug").Required().String()
clusterID = kingpin.Flag("clusterID", "Cluster UUID").Required().String()
tokenID = kingpin.Flag("tokenID", "Cluster token UUID").Required().String()
newDescription = kingpin.Flag("description", "New description for the cluster token").Required().String()
debug = kingpin.Flag("debug", "Enable debugging").Bool()
)

func main() {
Expand All @@ -26,12 +27,12 @@ func main() {
log.Fatalf("creating buildkite API client failed: %v", err)
}

clusterTokenUpdate := buildkite.ClusterTokenCreateUpdate{Description: "Dev squad agent token"}
clusterTokenUpdate := buildkite.ClusterTokenCreateUpdate{Description: *newDescription}

resp, err := client.ClusterTokens.Update(context.Background(), *org, *clusterID, *tokenID, clusterTokenUpdate)
token, _, err := client.ClusterTokens.Update(context.Background(), *org, *clusterID, *tokenID, clusterTokenUpdate)
if err != nil {
log.Fatalf("Updating cluster token failed: %s", err)
}

fmt.Println(resp.StatusCode)
fmt.Printf("Updated cluster token: %s\n", token.Description)
}
Loading

0 comments on commit a7e65d3

Please sign in to comment.