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.

Sign up for GitHub

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

Don’t redeploy twice the same manifest in a dev loop #843

Merged
merged 1 commit into from
Jul 25, 2018
Merged

Don’t redeploy twice the same manifest in a dev loop #843

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 32 additions & 4 deletions pkg/skaffold/deploy/kubectl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,9 @@ var warner Warner = &logrusWarner{}
type KubectlDeployer struct {
*v1alpha2.KubectlDeploy

workingDir string
kubeContext string
workingDir string
kubeContext string
previousDeployment manifestList
}

// NewKubectlDeployer returns a new KubectlDeployer for a DeployConfig filled
Expand Down Expand Up @@ -79,12 +80,18 @@ func (k *KubectlDeployer) Deploy(ctx context.Context, out io.Writer, builds []bu
return nil, errors.Wrap(err, "replacing images in manifests")
}

err = kubectl(manifests.reader(), out, k.kubeContext, k.Flags.Global, "apply", k.Flags.Apply, "-f", "-")
// Only redeploy modified or new manifests
// TODO(dgageot): should we delete a manifest that was deployed and is not anymore?
Copy link
Contributor

Choose a reason for hiding this comment

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

My opinion here would be no, because

  1. I like the experience of having a deployment recreated by skaffold if I delete it manually (read: accidentally) through kubectl/cloud console, and
  2. this would mean you could get skaffold into a state where it's not consistent with the configuration in the skaffold.yaml, which is a weird UX

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand. This PR will not anymore recreate deployments that you accidentally deleted. In exchange, dev cycles are much faster. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a tradeoff, but I guess if you do something silly like delete a deployment you could always just restart skaffold and have those deployments recreated. I could be convinced that speed is more important here.

updated := k.previousDeployment.diff(manifests)
logrus.Debugln(len(manifests), "manifests to deploy.", len(manifests), "are updated or new")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add space after deploy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, Debugln will add a space between values.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, TIL

k.previousDeployment = manifests

err = kubectl(updated.reader(), out, k.kubeContext, k.Flags.Global, "apply", k.Flags.Apply, "-f", "-")
if err != nil {
return nil, errors.Wrap(err, "deploying manifests")
}

return parseManifestsForDeploys(manifests)
return parseManifestsForDeploys(updated)
}

// Cleanup deletes what was deployed by calling Deploy.
Expand Down Expand Up @@ -223,6 +230,27 @@ func (l *manifestList) Empty() bool {
return len(*l) == 0
}

func (l *manifestList) diff(manifests manifestList) manifestList {
if l == nil {
return manifests
}

oldManifests := map[string]bool{}
for _, oldManifest := range *l {
oldManifests[string(oldManifest)] = true
}

var updated manifestList

for _, manifest := range manifests {
if !oldManifests[string(manifest)] {
updated = append(updated, manifest)
}
}

return updated
}

func (l *manifestList) reader() io.Reader {
return strings.NewReader(l.String())
}
Expand Down