Skip to content

Commit

Permalink
Merge pull request #797 from Pix4D/pci-3852-client-fix-save-config
Browse files Browse the repository at this point in the history 
go-concourse: SaveConfig endpoint: improve error message given to the end user
  • Loading branch information
@aliculPix4D
aliculPix4D authored Oct 8, 2024
2 parents a8c6204 + ac16027 commit 955cdea
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 35 deletions.
9 changes: 9 additions & 0 deletions fly/integration/set_pipeline_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ var _ = Describe("Fly CLI", func() {

Expect(receivedConfig).To(Equal(config))

w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusOK)
w.Write([]byte(`{}`))
},
Expand Down Expand Up @@ -341,6 +342,7 @@ var _ = Describe("Fly CLI", func() {
Expect(err).NotTo(HaveOccurred())

Expect(receivedConfig).To(Equal(config))
w.Header().Set("Content-Type", "application/json")

w.WriteHeader(http.StatusOK)
w.Write([]byte(`{}`))
Expand Down Expand Up @@ -885,6 +887,7 @@ this is super secure
ghttp.CombineHandlers(
ghttp.VerifyHeaderKV(atc.ConfigVersionHeader, "42"),
func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
config := getConfig(r)
Expect(config).To(MatchYAML(payload))
},
Expand Down Expand Up @@ -961,6 +964,7 @@ this is super secure
ghttp.CombineHandlers(
ghttp.VerifyHeaderKV(atc.ConfigVersionHeader, "42"),
func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
config := getConfig(r)
Expect(config).To(MatchYAML(payload))
},
Expand Down Expand Up @@ -1268,6 +1272,7 @@ this is super secure
atcServer.RouteToHandler("PUT", path, ghttp.CombineHandlers(
ghttp.VerifyHeaderKV(atc.ConfigVersionHeader, "42"),
func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
config := getConfig(r)
Expect(config).To(MatchYAML(payload))
},
Expand Down Expand Up @@ -1393,8 +1398,10 @@ this is super secure
ghttp.VerifyHeaderKV(atc.ConfigVersionHeader, "42"),
func(w http.ResponseWriter, r *http.Request) {
config := getConfig(r)
w.Header().Set("Content-Type", "application/json")
Expect(config).To(MatchYAML(payload))
},

ghttp.RespondWith(http.StatusOK, "{}"),
))

Expand All @@ -1419,6 +1426,7 @@ this is super secure
ghttp.VerifyHeaderKV(atc.ConfigVersionHeader, "42"),
func(w http.ResponseWriter, r *http.Request) {
config := getConfig(r)
w.Header().Set("Content-Type", "application/json")
Expect(config).To(MatchYAML(payload))
},
ghttp.RespondWith(http.StatusCreated, "{}"),
Expand All @@ -1443,6 +1451,7 @@ this is super secure
ghttp.VerifyHeaderKV(atc.ConfigVersionHeader, "42"),
func(w http.ResponseWriter, r *http.Request) {
config := getConfig(r)
w.Header().Set("Content-Type", "application/json")
Expect(config).To(MatchYAML(payload))
},
ghttp.RespondWith(http.StatusCreated, `{"warnings":[
Expand Down
19 changes: 17 additions & 2 deletions go-concourse/concourse/configs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package concourse
import (
"bytes"
"encoding/json"
"fmt"
"io"
"net/http"
"net/url"
Expand Down Expand Up @@ -85,18 +86,32 @@ func (team *team) CreateOrUpdatePipelineConfig(pipelineRef atc.PipelineRef, conf

switch response.StatusCode {
case http.StatusOK, http.StatusCreated:
if response.Header.Get("Content-Type") != "application/json" {
return false, false, []ConfigWarning{}, internal.UnexpectedResponseError{
StatusCode: response.StatusCode,
Status: response.Status,
Body: string(body),
}
}
configResponse := setConfigResponse{}
err = json.Unmarshal(body, &configResponse)
if err != nil {
return false, false, []ConfigWarning{}, err
return false, false, []ConfigWarning{}, fmt.Errorf("parsing JSON: %w", err)
}
created := response.StatusCode == http.StatusCreated
return created, !created, configResponse.Warnings, nil
case http.StatusBadRequest:
if response.Header.Get("Content-Type") != "application/json" {
return false, false, []ConfigWarning{}, internal.UnexpectedResponseError{
StatusCode: response.StatusCode,
Status: response.Status,
Body: string(body),
}
}
var validationErr atc.SaveConfigResponse
err = json.Unmarshal(body, &validationErr)
if err != nil {
return false, false, []ConfigWarning{}, err
return false, false, []ConfigWarning{}, fmt.Errorf("parsing JSON: %w", err)
}
return false, false, []ConfigWarning{}, InvalidConfigError{Errors: validationErr.Errors}
case http.StatusForbidden:
Expand Down
75 changes: 42 additions & 33 deletions go-concourse/concourse/configs_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,8 @@ var _ = Describe("ATC Handler Configs", func() {

Expect(receivedConfig).To(Equal(expectedConfig))

w.Header().Set("Content-Type", "application/json")

w.WriteHeader(returnHeader)
w.Write(returnBody)
},
Expand Down Expand Up @@ -237,17 +239,6 @@ var _ = Describe("ATC Handler Configs", func() {
}))
})

Context("when response contains bad JSON", func() {
BeforeEach(func() {
returnBody = []byte(`bad-json`)
})

It("returns an error", func() {
_, _, _, err := team.CreateOrUpdatePipelineConfig(pipelineRef, expectedVersion, expectedConfig, checkCredentials)
Expect(err).To(HaveOccurred())
})
})

Context("when instance vars are specified", func() {
BeforeEach(func() {
pipelineRef = atc.PipelineRef{
Expand Down Expand Up @@ -282,6 +273,26 @@ var _ = Describe("ATC Handler Configs", func() {
Expect(atcServer.ReceivedRequests()[0].URL.RawQuery).To(Equal("check_creds="))
})
})

Context("when response does not contain application/json header", func() {
BeforeEach(func() {
atcServer.RouteToHandler("PUT", "/api/v1/teams/some-team/pipelines/mypipeline/config",
ghttp.CombineHandlers(
ghttp.VerifyHeaderKV(atc.ConfigVersionHeader, "42"),
func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusOK)
w.Write([]byte(`server error`))
},
),
)
})

It("returns an error", func() {
_, _, _, err := team.CreateOrUpdatePipelineConfig(pipelineRef, expectedVersion, expectedConfig, checkCredentials)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("server error"))
})
})
})

Context("when updating a config", func() {
Expand Down Expand Up @@ -310,17 +321,6 @@ var _ = Describe("ATC Handler Configs", func() {
}))
})

Context("when response contains bad JSON", func() {
BeforeEach(func() {
returnBody = []byte(`bad-json`)
})

It("returns an error", func() {
_, _, _, err := team.CreateOrUpdatePipelineConfig(pipelineRef, expectedVersion, expectedConfig, checkCredentials)
Expect(err).To(HaveOccurred())
})
})

Context("when instance vars are specified", func() {
BeforeEach(func() {
pipelineRef = atc.PipelineRef{
Expand Down Expand Up @@ -369,17 +369,6 @@ var _ = Describe("ATC Handler Configs", func() {
Expect(err.Error()).To(ContainSubstring("invalid pipeline config:\n"))
Expect(err.Error()).To(ContainSubstring("fake-error1\nfake-error2"))
})

Context("when response contains bad JSON", func() {
BeforeEach(func() {
returnBody = []byte(`bad-json`)
})

It("returns an error", func() {
_, _, _, err := team.CreateOrUpdatePipelineConfig(pipelineRef, expectedVersion, expectedConfig, checkCredentials)
Expect(err).To(HaveOccurred())
})
})
})

Context("when setting config returns forbidden", func() {
Expand All @@ -394,5 +383,25 @@ var _ = Describe("ATC Handler Configs", func() {
Expect(err.Error()).To(ContainSubstring("forbidden: policy check failed: you can't do that"))
})
})

Context("when response does not contain application/json header", func() {
BeforeEach(func() {
atcServer.RouteToHandler("PUT", "/api/v1/teams/some-team/pipelines/mypipeline/config",
ghttp.CombineHandlers(
ghttp.VerifyHeaderKV(atc.ConfigVersionHeader, "42"),
func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusBadRequest)
w.Write([]byte(`server error`))
},
),
)
})

It("returns an error", func() {
_, _, _, err := team.CreateOrUpdatePipelineConfig(pipelineRef, expectedVersion, expectedConfig, checkCredentials)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("server error"))
})
})
})
})

0 comments on commit 955cdea

Please sign in to comment.