Simpler runner code by dgageot · Pull Request #540 · GoogleContainerTools/skaffold

dgageot · 2018-05-11T15:17:41Z

This is a first step in my master plan to simplify runner's code

Also fixes #559 and #516

dlorenc · 2018-05-11T15:40:15Z

This is a first step in my master plan to simplify runner's code

I like master plans! Could you share a little more info on the goal in here or an issue?

dgageot · 2018-05-11T15:55:32Z

@dlorenc So, next time I need to get your attention, I'll use the words "master plan" in the description.

I think runner.go has become too hard to read and change. I'd like to simplify it but each time I try, so this is me starting with baby steps. In parallel of making the code easier to read, I'd like to better handle fatal failures. Builds are ok to fail (maybe not the first time) but the rest shouldn't fail with only a warning.

r2d4 · 2018-05-23T18:08:29Z

pkg/skaffold/build/deps.go

+type DependencyMapFactory func(artifacts []*v1alpha2.Artifact) (DependencyMap, error)
+
+// DependencyMap is a bijection between artifacts and the files they depend on.
+type DependencyMap interface {


Whats the reasoning behind splitting this out into an interface if it only has one implementation?

Its a little odd that we have the DependencyMap and DependencyResolver interfaces. The latter is the one with the bazel, and dockerfile implementations.

Will remove the interface. It was useful at some point for unit test.

And in a future PR, I'll try to remove the need for DependencyResolver which is only required for tests.

r2d4 · 2018-05-23T18:09:59Z

pkg/skaffold/build/container_builder.go

 }

-func (cb *GoogleCloudBuilder) Build(ctx context.Context, out io.Writer, tagger tag.Tagger, artifacts []*v1alpha2.Artifact) (*BuildResult, error) {
+func (cb *GoogleCloudBuilder) Build(ctx context.Context, out io.Writer, tagger tag.Tagger, artifacts []*v1alpha2.Artifact) ([]Build, error) {


I'm fine getting rid of the build result - it didn't do anything to begin with.

r2d4 · 2018-05-23T18:19:00Z

pkg/skaffold/build/timings.go

+	Builder
+}
+
+func (w withTimings) Build(ctx context.Context, out io.Writer, tagger tag.Tagger, artifacts []*v1alpha2.Artifact) ([]Build, error) {


Formatting directives/notification bells probably shouldn't exist in the deploy/build packages. Maybe we can put them in runner and have the same withTiming struct embed both builder and deployer? I'm not sure if that would actually eliminate much code, but these implementations should as separate from the builders and deployers as possible.

r2d4 · 2018-05-23T18:40:42Z

pkg/skaffold/deploy/timings.go

+
+	err := w.Deployer.Deploy(ctx, out, builds)
+	if err != nil {
+		return err


nit wrap errors

I don't really like wrapping the error here since all we do is delegate to a function call without changing the inputs or outputs. It just adds noise to the errors I think.

r2d4 · 2018-05-23T18:43:00Z

pkg/skaffold/runner/runner.go

-	return err
+	bRes, err := r.Builder.Build(ctx, r.out, r.Tagger, r.config.Build.Artifacts)
+	if err != nil {
+		return err


nit: wrap error

r2d4 · 2018-05-23T20:08:24Z

pkg/skaffold/watch/watch.go

-			onChange(changes)
+
+			if err := onChange(changes); err != nil {
+				return err


nit: wrap error

r2d4 · 2018-05-23T20:15:05Z

pkg/skaffold/deploy/deploy.go

 	// Deploy should ensure that the build results are deployed to the Kubernetes
 	// cluster.
-	Deploy(context.Context, io.Writer, *build.BuildResult) (*Result, error)
+	Deploy(context.Context, io.Writer, []build.Build) error


r2d4 · 2018-05-23T20:21:25Z

pkg/skaffold/runner/runner.go


-	return nil, fmt.Errorf("Unknown tagger for strategy %s", t)
+	default:
+		return nil, fmt.Errorf("Unknown tagger for strategy %s", t)


nit: Unknown tagger for config %+v since its a struct now

r2d4 · 2018-05-23T20:35:49Z

pkg/skaffold/runner/runner.go

 func (r *SkaffoldRunner) Run(ctx context.Context) error {
-	_, _, err := r.buildAndDeploy(ctx, r.config.Build.Artifacts, nil)
-	return err
+	bRes, err := r.Builder.Build(ctx, r.out, r.Tagger, r.config.Build.Artifacts)


See other comment on func (r *SkaffoldRunner) Build(ctx context.Context) error {

r2d4 · 2018-05-23T20:37:09Z

pkg/skaffold/runner/runner.go

 }

 // Build builds the artifacts.
 func (r *SkaffoldRunner) Build(ctx context.Context) error {


Since we have this function, I think all r.Builder.Build should be translated to r.Build

I don't understand how we can do that. r.Build and r.Builder.Build are two different functions with same name but different arguments. I must be missing something.

The arguments passed into r.Builder.Build are just fields on the runner itself. So both functions end up passing the same args. Right now it looks like the runner's Build just is a wrapper around the embedded builder's Build with some pretty formatting.

I'm still not sure I know how to improve that. Can this be done in another PR?

Signed-off-by: David Gageot <david@gageot.net>

It’s only needed to show the logs. Fixes GoogleContainerTools#559 Signed-off-by: David Gageot <david@gageot.net>

Signed-off-by: David Gageot <david@gageot.net>

Fixes GoogleContainerTools#516 Signed-off-by: David Gageot <david@gageot.net>

dgageot · 2018-05-24T07:18:10Z

Hi @r2d4, I tried to take into account all your feedback

Signed-off-by: David Gageot <david@gageot.net>

dgageot requested review from dlorenc, r2d4 and viglesiasce as code owners May 11, 2018 15:17

dgageot force-pushed the simpler-builder-deployer branch 11 times, most recently from 7a4749e to 9321683 Compare May 17, 2018 16:45

dgageot changed the title ~~Simpler builder and deployer signatures~~ Simpler runner code May 17, 2018

dgageot force-pushed the simpler-builder-deployer branch 2 times, most recently from 9e1e2f7 to c1417e0 Compare May 21, 2018 16:34

r2d4 self-assigned this May 22, 2018

dgageot force-pushed the simpler-builder-deployer branch from c1417e0 to 055930e Compare May 23, 2018 07:51

r2d4 reviewed May 23, 2018

View reviewed changes

dgageot added 8 commits May 24, 2018 08:42

Remove unused deploy result

56ad6f1

Signed-off-by: David Gageot <david@gageot.net>

Simplify BuildResult

2f47ae1

Signed-off-by: David Gageot <david@gageot.net>

Improve runner’s code

f30f788

Signed-off-by: David Gageot <david@gageot.net>

Simplify test mocks

a613785

Signed-off-by: David Gageot <david@gageot.net>

Inline buildAndDeploy and better test Dev()

7873988

Signed-off-by: David Gageot <david@gageot.net>

Extract notification code

9de9579

Signed-off-by: David Gageot <david@gageot.net>

Extract timings code

5e81724

Improve warnings and errors

226ab71

Signed-off-by: David Gageot <david@gageot.net>

dgageot added 5 commits May 24, 2018 08:43

DepMap can be local

380f52f

Signed-off-by: David Gageot <david@gageot.net>

Lazy creation of the Kubernetes client

ef676f1

It’s only needed to show the logs. Fixes GoogleContainerTools#559 Signed-off-by: David Gageot <david@gageot.net>

Simplify tests

24b12cc

Signed-off-by: David Gageot <david@gageot.net>

Skaffold dev should stop on non build errors

c83fdbc

Fixes GoogleContainerTools#516 Signed-off-by: David Gageot <david@gageot.net>

Fixing details

4a6f3d3

dgageot force-pushed the simpler-builder-deployer branch from 055930e to 6f9f5c6 Compare May 24, 2018 07:17

dgageot mentioned this pull request May 24, 2018

Make sure deployed images are built while parsing skaffold config #581

Closed

Move timings and notifications to runner package

bfcdc22

Signed-off-by: David Gageot <david@gageot.net>

dgageot force-pushed the simpler-builder-deployer branch from 6f9f5c6 to bfcdc22 Compare May 24, 2018 07:30

r2d4 approved these changes May 24, 2018

View reviewed changes

dgageot merged commit 2fb273d into GoogleContainerTools:master May 24, 2018

dgageot deleted the simpler-builder-deployer branch May 29, 2018 04:49

r2d4 mentioned this pull request Jun 1, 2018

Unhelpful error: WARN[0001] run: build: build step: running push: denied: requested access to the resource is denied #507

Closed

Conversation

dgageot commented May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlorenc commented May 11, 2018

Uh oh!

dgageot commented May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgageot commented May 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dgageot commented May 11, 2018 •

edited

Loading

dgageot commented May 11, 2018 •

edited

Loading