Add Watch API and skaffold dev command
#15
Conversation
pkg/skaffold/config/config.go
Outdated
| @@ -77,6 +77,11 @@ type Artifact struct { | |||
| Workspace string `yaml:"workspace"` | |||
| } | |||
|
|
|||
| // DevelopmentConfig is the default development configuration for skaffold dev mode | |||
| var DevelopmentConfig = SkaffoldConfig{ | |||
| Watch: true, | |||
There was a problem hiding this comment.
Sorry just noticed this, but why is Watch on SkaffoldConfig? Would it ever make sense to have it set to true in the yaml itself?
There was a problem hiding this comment.
I'd like to use the config object itself to pass "dev mode" vs. "run mode" down to implementations.
While we could use a "Development: true" or something, why not expose some of the functionality to the user and let them toggle on/off development mode features in their skaffold.yaml?
I imagine that "development mode" would really just be a specific configuration of a skaffold.yaml that works great for development and has features turned on that you wouldn't want to use in CI/CD.
pkg/skaffold/watch/watch.go
Outdated
| ) | ||
|
|
||
| //TODO(@r2d4): Figure out best UX to support configuring this blacklist | ||
| var ignoredPrefixes = []string{"vendor", ".git"} |
There was a problem hiding this comment.
I'd like to leave this as an open question and give it a lot more thought. Some preliminary ideas:
- .skaffoldignore
- .gitignore (the idea behind this is that if you're doing real git ops, you shouldn't rely on things outside your repo for the build process)
- skaffold.yaml parameters
We might need a combination of these
There was a problem hiding this comment.
Yeah definitely. Probably worth opening an issue to discuss alternatives.
pkg/skaffold/watch/watch.go
Outdated
| // We need to evaluate the symlink if the dependency is a symlink | ||
| // inotify will return the final path of the symlink | ||
| // so we need to match it to know which dockerfile needs rebuilding | ||
| evalPath, err := filepath.EvalSymlinks(dep) |
There was a problem hiding this comment.
Should this be conditional on whether or not it is a symlink? Can we do a stat to see?
There was a problem hiding this comment.
Ah yeah, os.Stat should give us a fileinfo that will let us see fileinfo.Mode().IsRegular()
However, should docker.GetDockerfileDependencies be responsible for evaluating the possible symlinks anyways?
pkg/skaffold/watch/watch.go
Outdated
| c := make(chan notify.EventInfo, 1) | ||
| errCh := make(chan error, 1) | ||
| defer notify.Stop(c) | ||
| for _, a := range artifacts { |
There was a problem hiding this comment.
For testability it might be nicer to break this into a function that returns a set of deps per artifact, then a function to add the watches on that.
There was a problem hiding this comment.
Thats much cleaner, I'll try to refactor it that way
Purely aesthetic, but it makes the info level debugging much more easily parsed.
pkg/skaffold/watch/watch_test.go
Outdated
| } | ||
| } | ||
|
|
||
| cleanup() |
There was a problem hiding this comment.
Why do we cleanup before the start? Doesn't ioutil.TempDir give us a clean dir?
pkg/skaffold/watch/watch.go
Outdated
| select { | ||
| case ei := <-c: | ||
| logrus.Infof("%s %s", ei.Event().String(), ei.Path()) | ||
| artifact := depsToArtifact[ei.Path()] |
There was a problem hiding this comment.
I might be missing something, but do we need a loop here? Can one dep be part of multiple artifacts?
There was a problem hiding this comment.
removed the loop, added slice of changed artifacts
|
ref #36 |
|
Rebased on the last merges, should be ready for another look @dlorenc |
pkg/skaffold/watch/watch.go
Outdated
| } | ||
| } | ||
| if ready != nil { | ||
| ready <- WatchStartEvent |
There was a problem hiding this comment.
A logging statement right before this could come in handy.
pkg/skaffold/watch/watch.go
Outdated
| if err != nil { | ||
| return errors.Wrapf(err, "stat %s", dep) | ||
| } | ||
| if !fi.Mode().IsRegular() { |
There was a problem hiding this comment.
Nit: can you explicitly check for symlink instead of "!regular"?
What about directories, devices...
Fixes #11
Fixes #13