Add a Build command#476
Conversation
r2d4
left a comment
r2d4
left a comment
There was a problem hiding this comment.
First pass. I'm actually really excited for exposing these subcommands. Might have to wait for after kubecon to merge, but looks pretty good.
| } | ||
| return r.dev(ctx) | ||
| // Build builds the artifacts. | ||
| func (r *SkaffoldRunner) Build(ctx context.Context) error { |
There was a problem hiding this comment.
Should skaffold build or skaffold deploy call the builders and deployers directly? Theoretically, they should only do exactly that, and the CLI output UX could be handled in cmd.
Right now I guess that would mean exposing getBuilder or getDeployer, which should hopefully go away with #388
There was a problem hiding this comment.
Since the builder is embedded in the runner, you can just call build straight from that. Then, our cmd/build would just look like
runner := NewForConfig()
runner.Build()
Or most likely you can reuse the private r.build here so that the UX looks similar.
There was a problem hiding this comment.
I'd rather try that in another PR. I've tried something similar and failed.
cmd/skaffold/app/cmd/build.go
Outdated
| Short: "Builds the artifacts", | ||
| Args: cobra.NoArgs, | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| return runSkaffold(out, filename, func(ctx context.Context, r *runner.SkaffoldRunner) error { |
There was a problem hiding this comment.
I'm not sure I have a better idea right now, but the signature of this function makes it feel like its a bit overloaded.
Its toggling between run (build + deploy), dev (build + deploy loop), and build.
I'd be fine just having a function that returns the runner and call run, dev, or build here.
There was a problem hiding this comment.
Good point! I'll just return the runner
| func (r *SkaffoldRunner) Build(ctx context.Context) error { | ||
| bRes, err := r.build(ctx, r.config.Build.Artifacts) | ||
|
|
||
| for _, res := range bRes.Builds { |
There was a problem hiding this comment.
This could live in r.build and be exposed there. Or if you dont think its appropriate for dev/run mode, we can add it in cmd/
There was a problem hiding this comment.
Build and build don't take the same parameters. I'd rather try to refactor that in a separate PR
| } | ||
|
|
||
| func GetConfig(contents []byte, useDefault bool, dev bool) (util.VersionedConfig, error) { | ||
| func GetConfig(contents []byte, useDefault bool) (util.VersionedConfig, error) { |
There was a problem hiding this comment.
Does this also mean that we no longer have default differences between dev/run? I'm ok with that if thats the case, but it should be noted.
| } else { | ||
| cfg.Build.TagPolicy = TagPolicy{GitTagger: &GitTagger{}} | ||
| } | ||
| cfg.Build.TagPolicy = TagPolicy{GitTagger: &GitTagger{}} |
There was a problem hiding this comment.
Yes! That's in the commit message. I've added that to the PR description. It greatly simplify the code and makes more sense for the users.
|
@r2d4 I've applied your feedback and would love to refactor the Dev/Build/Run functions on a separate PR. It requires refactoring a lot of the tests too. |
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
Fixes GoogleContainerTools#425 Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
3 participants
Also fixes #425, which means there are no more different defaults in dev and run modes.