Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add validations to Control API for Auto Triggers #4242

Merged
merged 3 commits into from
Jun 3, 2020

Conversation

gsquared94
Copy link
Contributor

@gsquared94 gsquared94 commented May 21, 2020

Add validations to Control API for Auto Triggers

Related: #4145

Description
User facing changes (remove if N/A)

Return specific error codes with message

$ curl -X PUT http://localhost:50052/v1/build/auto_execute -d '{"enabled":false}' -i
HTTP/1.1 409 Conflict
Content-Type: application/json
Date: Fri, 29 May 2020 23:53:47 GMT
Content-Length: 47

{"error":"auto build is already set to false"}


$ curl -X PUT http://localhost:50052/v1/build/auto_execute -d '{"enable2d":false}' -i
HTTP/1.1 400 Bad Request
Content-Type: application/json
Date: Fri, 29 May 2020 23:54:40 GMT
Content-Length: 57

{"error":"missing required boolean parameter 'enabled'"}


$ curl -X PUT http://localhost:50052/v1/build/auto_execute -d '{"enabled":1}' -i
HTTP/1.1 400 Bad Request
Content-Type: application/json
Date: Fri, 29 May 2020 23:54:50 GMT
Content-Length: 69

{"error":"json: cannot unmarshal number into Go value of type bool"}

@codecov
Copy link

codecov bot commented May 21, 2020

Codecov Report

Merging #4242 into master will increase coverage by 0.21%.
The diff coverage is 4.00%.

Impacted Files Coverage Δ
pkg/skaffold/server/endpoints.go 0.00% <0.00%> (ø)
pkg/skaffold/server/server.go 48.88% <11.11%> (-4.77%) ⬇️
pkg/skaffold/kubernetes/watcher.go 87.50% <0.00%> (-12.50%) ⬇️
...affold/kubernetes/portforward/kubectl_forwarder.go 63.41% <0.00%> (-4.88%) ⬇️
...ffold/kubernetes/portforward/resource_forwarder.go 84.31% <0.00%> (-3.45%) ⬇️
...g/skaffold/kubernetes/portforward/pod_forwarder.go 82.75% <0.00%> (-3.18%) ⬇️
pkg/skaffold/build/buildpacks/init.go 91.30% <0.00%> (-1.01%) ⬇️
pkg/skaffold/deploy/helm.go 79.83% <0.00%> (-0.41%) ⬇️
pkg/skaffold/schema/versions.go 82.75% <0.00%> (ø)
... and 15 more

@gsquared94 gsquared94 added the kokoro:run runs the kokoro jobs on a PR label May 21, 2020
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label May 21, 2020
Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! this is pretty clean, and works well. my only comment is that in the CLI, i get errors that look like:

{"error":"auto build is already set to true","code":6,"message":"auto build is already set to true"}%

{"error":"json: cannot unmarshal bool into Go value of type map[string]json.RawMessage","code" :3,"message":"json: cannot unmarshal bool into Go value of type map[string]json.RawMessage"}%

could you remove the duplication of the error message to make it more readable?

proto/skaffold.proto Show resolved Hide resolved
@gsquared94
Copy link
Contributor Author

nice! this is pretty clean, and works well. my only comment is that in the CLI, i get errors that look like:

{"error":"auto build is already set to true","code":6,"message":"auto build is already set to true"}%

{"error":"json: cannot unmarshal bool into Go value of type map[string]json.RawMessage","code" :3,"message":"json: cannot unmarshal bool into Go value of type map[string]json.RawMessage"}%

could you remove the duplication of the error message to make it more readable?

Done

@gsquared94 gsquared94 closed this May 30, 2020
@gsquared94 gsquared94 reopened this May 30, 2020
@gsquared94 gsquared94 requested a review from nkubala May 30, 2020 00:03
@gsquared94 gsquared94 added the kokoro:run runs the kokoro jobs on a PR label May 30, 2020
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label May 30, 2020
@nkubala nkubala merged commit 60b9a1e into GoogleContainerTools:master Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants