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

Expose control API for builds, syncs, and deploys #2450

Merged
merged 1 commit into from
Jul 22, 2019

Conversation

nkubala
Copy link
Contributor

@nkubala nkubala commented Jul 11, 2019

this change introduces an API for controlling the individual components (build, sync, and deploy) of Skaffold. this API can be accessed through the gRPC/HTTP servers that skaffold is already starting up when in dev mode.

each component can be set to run in "api mode" through its own flag (--build-trigger=api, --deploy-trigger=api, --sync-trigger=api) leaving the others to run as normal. optionally, the --api-mode=true flag can be set, which implicitly sets all triggers to api. this flag serves only to eliminate verbosity on the command line.

when in "api mode", a component's action will not be performed until a user sends an "intent" to the skaffold server. multiple intents can be sent at one time, for example a "build" and "deploy" can be sent simultaneously. the ordering of these actions is still preserved in the dev loop logic. changes to the local filesystem are still computed as tracked as normal; skaffold "remembers" them until the corresponding user intent is received.

examples of user intent being sent to skaffold:

RPC

conn, _ = grpc.Dial(fmt.Sprintf(":%s", skaffoldRPCPort), grpc.WithInsecure())
client = proto.NewSkaffoldServiceClient(conn)
client.Execute(context.Background(), &proto.UserIntentRequest{
    Intent: &proto.Intent{
          Build: true,
          Deploy: true,
    },
})

HTTP
curl http://localhost:<skaffold_http_port>/v1/execute -d '{"build": true, "deploy": true}'

Fixes #1556
Fixes #1520
Fixes #1396

@codecov
Copy link

codecov bot commented Jul 11, 2019

Codecov Report

Merging #2450 into master will increase coverage by 0.01%.
The diff coverage is 65.53%.

Impacted Files Coverage Δ
pkg/skaffold/config/options.go 100% <ø> (ø) ⬆️
pkg/skaffold/trigger/triggers.go 30.95% <ø> (+1.82%) ⬆️
pkg/skaffold/runner/listen.go 0% <0%> (ø) ⬆️
pkg/skaffold/server/endpoints.go 0% <0%> (ø) ⬆️
cmd/skaffold/app/cmd/dev.go 86.66% <100%> (+2.05%) ⬆️
pkg/skaffold/runner/intent.go 100% <100%> (ø)
pkg/skaffold/server/server.go 58.57% <35.71%> (-7.54%) ⬇️
pkg/skaffold/runner/runner.go 69.56% <70.58%> (-0.73%) ⬇️
pkg/skaffold/runner/changeset.go 84% <77.77%> (-16%) ⬇️
pkg/skaffold/runner/dev.go 66.32% <81.48%> (+2.69%) ⬆️
... and 3 more

@nkubala nkubala force-pushed the control-api branch 2 times, most recently from b67a001 to 32bafd1 Compare July 11, 2019 22:33
pkg/skaffold/runner/dev.go Outdated Show resolved Hide resolved
@nkubala nkubala force-pushed the control-api branch 2 times, most recently from 5f5d7cd to 1a550cb Compare July 12, 2019 19:42
pkg/skaffold/runner/runner.go Outdated Show resolved Hide resolved
pkg/skaffold/runner/dev.go Outdated Show resolved Hide resolved
@balopat
Copy link
Contributor

balopat commented Jul 12, 2019

Other than the last couple of comments I think this is looking pretty good actually 👍

cmd/skaffold/app/cmd/dev.go Outdated Show resolved Hide resolved
pkg/skaffold/runner/runner.go Outdated Show resolved Hide resolved
pkg/skaffold/runner/runner.go Outdated Show resolved Hide resolved
@nkubala nkubala force-pushed the control-api branch 2 times, most recently from c8077b2 to 6817e85 Compare July 16, 2019 18:44
@nkubala
Copy link
Contributor Author

nkubala commented Jul 16, 2019

unfortunately this design isn't working with the notify trigger as is. once fsnotify tells skaffold something has changed, it kicks off a dev loop, but if skaffold hasn't received a user intent to build, the dev loop will exit...and won't try again until fsnotify detects a new change :( working on a fix for this now.

update: this should be working with the notify trigger now. added an intent channel that the server sends back on and is subscribed to by the listener, as an alternative entrypoint into the dev loop

cmd/skaffold/app/cmd/dev.go Outdated Show resolved Hide resolved
cmd/skaffold/app/cmd/dev.go Outdated Show resolved Hide resolved
integration/dev_test.go Outdated Show resolved Hide resolved
integration/sync_test.go Outdated Show resolved Hide resolved
pkg/skaffold/config/options.go Outdated Show resolved Hide resolved
pkg/skaffold/runner/runner_test.go Outdated Show resolved Hide resolved
pkg/skaffold/runner/intent.go Show resolved Hide resolved
pkg/skaffold/runner/runner.go Show resolved Hide resolved
pkg/skaffold/runner/runner.go Outdated Show resolved Hide resolved
pkg/skaffold/server/server.go Outdated Show resolved Hide resolved
Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

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

LGTM!! @dgageot ?

Copy link
Contributor

@dgageot dgageot left a comment

Choose a reason for hiding this comment

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

I would merge after the release of 0.34 to give us time to test well

@nkubala nkubala merged commit 6ddb860 into GoogleContainerTools:master Jul 22, 2019
@nkubala nkubala deleted the control-api branch July 22, 2019 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants