Skip to content

Commit

Permalink
address review comments
Browse files Browse the repository at this point in the history 
Signed-off-by: aliculPix4D <[email protected]>
  • Loading branch information
@aliculPix4D
aliculPix4D committed Oct 8, 2024
1 parent 3cf91e9 commit d5a5639
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 86 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
32 changes: 15 additions & 17 deletions go-concourse/concourse/configs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,36 +86,34 @@ func (team *team) CreateOrUpdatePipelineConfig(pipelineRef atc.PipelineRef, conf

switch response.StatusCode {
case http.StatusOK, http.StatusCreated:
if response.Header.Get("Content-Type") == "application/json" {
configResponse := setConfigResponse{}
err = json.Unmarshal(body, &configResponse)
if err != nil {
return false, false, []ConfigWarning{}, fmt.Errorf("client: unable to JSON parse the response body: %w", err)
}
created := response.StatusCode == http.StatusCreated
return created, !created, configResponse.Warnings, nil
} else {
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{}, 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" {
var validationErr atc.SaveConfigResponse
err = json.Unmarshal(body, &validationErr)
if err != nil {
return false, false, []ConfigWarning{}, fmt.Errorf("client: unable to JSON parse the response body: %w", err)
}
return false, false, []ConfigWarning{}, InvalidConfigError{Errors: validationErr.Errors}
} else {
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{}, fmt.Errorf("parsing JSON: %w", err)
}
return false, false, []ConfigWarning{}, InvalidConfigError{Errors: validationErr.Errors}
case http.StatusForbidden:
return false, false, []ConfigWarning{}, internal.ForbiddenError{
Reason: string(body),
Expand Down
110 changes: 41 additions & 69 deletions go-concourse/concourse/configs_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,6 @@ var _ = Describe("ATC Handler Configs", func() {
expectedVersion string
expectedConfig []byte

contentType string
returnHeader int
returnBody []byte

Expand Down Expand Up @@ -205,9 +204,7 @@ var _ = Describe("ATC Handler Configs", func() {

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

if contentType != "" {
w.Header().Set("Content-Type", contentType)
}
w.Header().Set("Content-Type", "application/json")

w.WriteHeader(returnHeader)
w.Write(returnBody)
Expand All @@ -218,7 +215,6 @@ var _ = Describe("ATC Handler Configs", func() {

Context("when creating a new config", func() {
BeforeEach(func() {
contentType = "application/json"
returnHeader = http.StatusCreated
returnBody = []byte(`{"warnings":[
{"type": "warning-1-type", "message": "fake-warning1"},
Expand All @@ -243,31 +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())
Expect(err.Error()).To(ContainSubstring("client: unable to JSON parse the response body"))
})
})

Context("when response does not contain application/json header", func() {
BeforeEach(func() {
contentType = "text/plain"
returnBody = []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 instance vars are specified", func() {
BeforeEach(func() {
pipelineRef = atc.PipelineRef{
Expand Down Expand Up @@ -302,11 +273,30 @@ 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() {
BeforeEach(func() {
contentType = "application/json"
returnHeader = http.StatusOK
returnBody = []byte(`{"warnings":[
{"type": "warning-1-type", "message": "fake-warning1"},
Expand All @@ -331,18 +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())
Expect(err.Error()).To(ContainSubstring("client: unable to JSON parse the response body"))
})
})

Context("when instance vars are specified", func() {
BeforeEach(func() {
pipelineRef = atc.PipelineRef{
Expand Down Expand Up @@ -381,7 +359,6 @@ var _ = Describe("ATC Handler Configs", func() {

Context("when setting config returns bad request", func() {
BeforeEach(func() {
contentType = "application/json"
returnHeader = http.StatusBadRequest
returnBody = []byte(`{"errors":["fake-error1","fake-error2"]}`)
})
Expand All @@ -392,31 +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())
Expect(err.Error()).To(ContainSubstring("client: unable to JSON parse the response body"))
})
})

Context("when response does not contain application/json header", func() {
BeforeEach(func() {
contentType = "text/plain"
returnBody = []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 setting config returns forbidden", func() {
Expand All @@ -431,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 d5a5639

Please sign in to comment.