-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Simpler runner code #540
Simpler runner code #540
Conversation
I like master plans! Could you share a little more info on the goal in here or an issue? |
@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. |
7a4749e
to
9321683
Compare
9e1e2f7
to
c1417e0
Compare
c1417e0
to
055930e
Compare
pkg/skaffold/build/deps.go
Outdated
type DependencyMapFactory func(artifacts []*v1alpha2.Artifact) (DependencyMap, error) | ||
|
||
// DependencyMap is a bijection between artifacts and the files they depend on. | ||
type DependencyMap interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whats the reasoning behind splitting this out into an interface if it only has one implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a little odd that we have the DependencyMap and DependencyResolver interfaces. The latter is the one with the bazel, and dockerfile implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove the interface. It was useful at some point for unit test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And in a future PR, I'll try to remove the need for DependencyResolver which is only required for tests.
@@ -75,21 +75,24 @@ func NewGoogleCloudBuilder(cfg *v1alpha2.BuildConfig) (*GoogleCloudBuilder, erro | |||
return &GoogleCloudBuilder{cfg}, nil | |||
} | |||
|
|||
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine getting rid of the build result - it didn't do anything to begin with.
pkg/skaffold/build/timings.go
Outdated
Builder | ||
} | ||
|
||
func (w withTimings) Build(ctx context.Context, out io.Writer, tagger tag.Tagger, artifacts []*v1alpha2.Artifact) ([]Build, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pkg/skaffold/deploy/timings.go
Outdated
|
||
err := w.Deployer.Deploy(ctx, out, builds) | ||
if err != nil { | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit wrap errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
pkg/skaffold/runner/runner.go
Outdated
return err | ||
bRes, err := r.Builder.Build(ctx, r.out, r.Tagger, r.config.Build.Artifacts) | ||
if err != nil { | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: wrap error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
pkg/skaffold/watch/watch.go
Outdated
onChange(changes) | ||
|
||
if err := onChange(changes); err != nil { | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: wrap error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
// Deployer is the Deploy API of skaffold and responsible for deploying | ||
// the build results to a Kubernetes cluster | ||
type Deployer interface { | ||
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
pkg/skaffold/runner/runner.go
Outdated
|
||
return nil, fmt.Errorf("Unknown tagger for strategy %s", t) | ||
default: | ||
return nil, fmt.Errorf("Unknown tagger for strategy %s", t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Unknown tagger for config %+v
since its a struct now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
} | ||
|
||
return nil | ||
} | ||
|
||
// Run runs the skaffold build and deploy pipeline. | ||
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See other comment on func (r *SkaffoldRunner) Build(ctx context.Context) error {
return nil, fmt.Errorf("Unknown tagger for strategy %s", t) | ||
default: | ||
return nil, fmt.Errorf("Unknown tagger for strategy %s", t) | ||
} | ||
} | ||
|
||
// Build builds the artifacts. | ||
func (r *SkaffoldRunner) Build(ctx context.Context) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have this function, I think all r.Builder.Build should be translated to r.Build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
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>
055930e
to
6f9f5c6
Compare
Hi @r2d4, I tried to take into account all your feedback |
Signed-off-by: David Gageot <david@gageot.net>
6f9f5c6
to
bfcdc22
Compare
This is a first step in my master plan to simplify runner's code
Also fixes #559 and #516