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 control API to pause and resume autoBuild, autoDeploy and autoSync #4145

Merged
merged 15 commits into from May 20, 2020

Conversation

gsquared94
Copy link
Collaborator

@gsquared94 gsquared94 commented May 11, 2020

Fixes: #2806
Description

Add control API to pause and resume autoBuild, autoDeploy and autoSync
User facing changes (remove if N/A)

API Contract
image

@codecov
Copy link

codecov bot commented May 11, 2020

Codecov Report

Merging #4145 into master will decrease coverage by 0.43%.
The diff coverage is 43.05%.

Impacted Files Coverage Δ
pkg/skaffold/server/endpoints.go 0.00% <0.00%> (ø)
pkg/skaffold/server/server.go 53.65% <25.00%> (-4.92%) ⬇️
pkg/skaffold/runner/new.go 64.51% <42.85%> (-3.82%) ⬇️
pkg/skaffold/runner/intent.go 79.31% <70.00%> (-20.69%) ⬇️
pkg/skaffold/event/event.go 90.52% <71.42%> (-2.01%) ⬇️

@gsquared94 gsquared94 marked this pull request as ready for review May 12, 2020 20:46
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.

I tried this out locally, and I'm seeing strange behavior with the microservices example. when I set auto-deploy to false with curl -X POST http://localhost:50052/v1/auto_execute -d '{"deploy": false}', changes aren't triggering a rebuild when they should be. at first glance I don't see any copy paste errors, but can you try this example out and see if you're seeing the same issue?

also, it would be great if we could expose the state of each "switch" in the exposed state object. sometimes I lose track of where I am, and it would be nice to have a way to tell. this would also be useful for the IDEs to integrate this so they wouldn't have to keep track in the client (though they may anyway). this could come in a follow up PR though

@gsquared94
Copy link
Collaborator Author

gsquared94 commented May 14, 2020

I tried this out locally, and I'm seeing strange behavior with the microservices example. when I set auto-deploy to false with curl -X POST http://localhost:50052/v1/auto_execute -d '{"deploy": false}', changes aren't triggering a rebuild when they should be. at first glance I don't see any copy paste errors, but can you try this example out and see if you're seeing the same issue?

I think that's because {"deploy": false} is equivalent to {"build": false, "sync": false, "deploy": false} since the missing parameters in rpc request is set to default value AFAIK. That's something I wanted your opinion on. Either we use struct params (instead of boolean) in Intent otherwise we'll need 3 API's for the three phases separately. Which do you think is better?

@gsquared94
Copy link
Collaborator Author

also, it would be great if we could expose the state of each "switch" in the exposed state object. sometimes I lose track of where I am, and it would be nice to have a way to tell. this would also be useful for the IDEs to integrate this so they wouldn't have to keep track in the client (though they may anyway). this could come in a follow up PR though

It should be there as AutoTrigger property in BuildState, DeployState and FileSyncState already. The API doesn't return params if they are their default value. So if you ran with auto-build=false then the state api response won't show AutoTrigger param for BuildState.

@gsquared94 gsquared94 requested a review from nkubala May 14, 2020 19:55
@nkubala nkubala self-assigned this May 18, 2020
# Conflicts:
#	integration/dev_test.go
#	pkg/skaffold/runner/new.go
@gsquared94 gsquared94 closed this May 19, 2020
@gsquared94 gsquared94 added the kokoro:run runs the kokoro jobs on a PR label May 20, 2020
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label May 20, 2020
@gsquared94 gsquared94 dismissed nkubala’s stale review May 20, 2020 16:22

Review comments addressed

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.

this is looking great! just tried it out locally and it works exactly how i'd expect. my only comment is i think it would be nice if the server was able to tell me when i'm providing a malformed response or an unsupported HTTP method - I was trying POST for a while, and malformed payloads as well, and the server just responds with {}. it would also be nice if the server could tell me if i'm trying to flip the trigger to a position it's already in, e.g. if we've already turned off auto-build, and we try to again, the server could send a quick message saying "auto building is already disabled" or something.

these are just improvements though, and could come in follow up PRs.

@gsquared94 gsquared94 merged commit 3a50d01 into GoogleContainerTools:master May 20, 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.

Pause continuous deployment
4 participants