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 API build trigger #2201

Merged
merged 4 commits into from
Jun 4, 2019

Conversation

nkubala
Copy link
Contributor

@nkubala nkubala commented May 29, 2019

This adds an API build trigger type, which allows users to use the gRPC/HTTP server in skaffold to trigger builds in place of the polling/manual triggers.

When using the API trigger in dev mode, skaffold will only rebuild images if a trigger is issued by the user (and if files have changed since the last rebuild).

@codecov-io
Copy link

codecov-io commented May 29, 2019

Codecov Report

Merging #2201 into master will decrease coverage by 0.41%.
The diff coverage is 24.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2201      +/-   ##
==========================================
- Coverage   58.56%   58.14%   -0.42%     
==========================================
  Files         188      188              
  Lines        7792     7863      +71     
==========================================
+ Hits         4563     4572       +9     
- Misses       2858     2919      +61     
- Partials      371      372       +1
Impacted Files Coverage Δ
pkg/skaffold/event/event.go 89.65% <ø> (ø) ⬆️
pkg/skaffold/server/proto/skaffold.pb.gw.go 7.35% <0%> (ø)
pkg/skaffold/server/endpoints.go 0% <0%> (ø) ⬆️
pkg/skaffold/server/proto/skaffold.pb.go 5.66% <0%> (ø)
pkg/skaffold/runner/context/context.go 62.5% <100%> (+1.2%) ⬆️
pkg/skaffold/server/server.go 54.68% <100%> (+3.02%) ⬆️
pkg/skaffold/runner/runner.go 69.1% <100%> (ø) ⬆️
pkg/skaffold/watch/triggers.go 45.65% <37.5%> (-6.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81a56b1...b429454. Read the comment docs.

@tejal29
Copy link
Contributor

tejal29 commented May 30, 2019

This adds an API build trigger type, which allows users to use the gRPC/HTTP server in skaffold to trigger builds in place of the polling/manual triggers.

Can you give an example how this works?
e.g.: will there be a skaffold service started locally and then watch for triggers?
How do you start the gRPC/HTTP server?

Or is this part of skaffold dev flow only because that is when gRPC and HTTP servers are started?

@nkubala
Copy link
Contributor Author

nkubala commented May 30, 2019

Or is this part of skaffold dev flow only because that is when gRPC and HTTP servers are started?

exactly, sorry I wasn't very descriptive here. the default trigger is polling which means "the watcher will poll for file changes every X milliseconds, and rebuild if it finds any". this adds a new api trigger type which means "the watcher will poll for file changes only when the user tells it to". so in practice it would look like

$ skaffold dev --trigger api

...
make some file changes...

$ curl http://localhost:<skaffold_http_port>/v1/build

users can also use a gRPC client instead of curl and achieve the same result.

conn, err := grpc.Dial(skaffoldRPCAddr, grpc.WithInsecure())
if err != nil {
	fmt.Printf("error opening connection: %s\n", err.Error())
	os.Exit(1)
}
defer conn.Close()
client := proto.NewSkaffoldServiceClient(conn)
_, err := client.Build(ctx, &empty.Empty{})
if err != nil {
	fmt.Printf("error issuing build request: %v\n", err)
}

Copy link
Contributor

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

Overall this looks good. Just few clarifications.

pkg/skaffold/server/proto/skaffold.proto Show resolved Hide resolved
pkg/skaffold/watch/triggers.go Show resolved Hide resolved
pkg/skaffold/watch/triggers_test.go Outdated Show resolved Hide resolved
pkg/skaffold/server/server_test.go Outdated Show resolved Hide resolved
pkg/skaffold/watch/triggers.go Show resolved Hide resolved
pkg/skaffold/watch/triggers.go Outdated Show resolved Hide resolved
pkg/skaffold/watch/triggers.go Outdated Show resolved Hide resolved
@nkubala nkubala force-pushed the control-api-rebased branch from e3eb796 to cd0e10b Compare May 30, 2019 18:13
@tejal29
Copy link
Contributor

tejal29 commented May 30, 2019

This looks great!
I was thinking the Control API is per stage and i was wondering if there are future plans to specify it per stage i.e. "build", "test" and "deploy".
An example use-case would be:
I am k8 developer developing running skaffold dev. I make some changes to my build code and now my container structure test fail as expected.
I change my tests (e.g. config file for container structure test). In this case, it would be sufficient to trigger just the tests and no deploy.
(i am aware build will probably be a no-op since no build files were changed)

Another example:
I changed on the manifest which is deploying a 3rd party container image. In that case, i want to trigger just deploy.

Not to be tackled in this PR, but would be great to know if we plan to do this future.

Copy link
Contributor

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

LGTM except a nit on apiServer.Interval

@nkubala
Copy link
Contributor Author

nkubala commented May 30, 2019

@tejal29 spot on, I'll be sending a follow up PR that adds a trigger for deploys as well. the idea is to split the dev loop into its individual components and give users the ability to control each one independently (if they want to).

I haven't figured out the story for testing yet though: I originally considered it to be part of the build step, but your example makes a good point. I suppose users could specify their test files as build dependencies for the artifact they're testing, but I'll think more about it.

@nkubala nkubala dismissed balopat’s stale review May 30, 2019 20:21

addressed comments

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.

looking good, I have only one concern left

pkg/skaffold/runner/context/context.go Show resolved Hide resolved
@nkubala nkubala force-pushed the control-api-rebased branch from b20c50d to b429454 Compare June 3, 2019 23:28
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

@balopat balopat merged commit f4c601b into GoogleContainerTools:master Jun 4, 2019
@nkubala nkubala deleted the control-api-rebased branch October 17, 2019 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants