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

Jump to bottom

Lists files recursively in jib.getDependencies and other fixes. #1097

Merged
merged 9 commits into from
Oct 8, 2018
Merged

Lists files recursively in jib.getDependencies and other fixes. #1097

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a5775f1
Lists files recursively in jib.getDependencies.
coollog Oct 4, 2018
a6f4ddc
goimports
coollog Oct 4, 2018
d566548
Account for JibGradleArtifact.Project.
coollog Oct 4, 2018
16007c3
goimports
coollog Oct 4, 2018
396fbb2
Merge branch 'jib_skaffold' into jib-walk-deps
coollog Oct 4, 2018
5374868
Removes debug message.
coollog Oct 4, 2018
1e643fc
Errors if trying to use Maven multi-module.
coollog Oct 4, 2018
d0c069d
Merge branch 'jib-unified' into jib-walk-deps
coollog Oct 8, 2018
30ce7ed
Merge branch 'jib_skaffold' into jib-walk-deps
coollog Oct 8, 2018
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
33 changes: 29 additions & 4 deletions pkg/skaffold/jib/jib.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,12 @@ import (
"os/exec"
"strings"

"os"
"path/filepath"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
)

func getDependencies(cmd *exec.Cmd) ([]string, error) {
Expand All @@ -30,14 +35,34 @@ func getDependencies(cmd *exec.Cmd) ([]string, error) {
}

// Parses stdout for the dependencies, one per line
// TODO(coollog) directories should be expanded recursively
lines := strings.Split(string(stdout), "\n")
var deps []string
for _, l := range lines {
if l == "" {
for _, dep := range lines {
if dep == "" {
continue
}

// Resolves directories recursively.
info, err := os.Stat(dep)
if err != nil {
if os.IsNotExist(err) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is modeled after the logic in changes.go#stat. I think ignoring non-existent files should be fine here since the files would be ignored by stat anyways. The alternative is to just add the file to the deps list upon this os.Stat error. Thoughts?

logrus.Debugf("could not stat dependency: %s", err)
continue // Ignore files that don't exist
}
return nil, errors.Wrapf(err, "unable to stat file %s", dep)
}
if info.IsDir() {
err := filepath.Walk(dep, func(path string, info os.FileInfo, err error) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

godirwalk.Walk is much faster at walking directories (like 10x faster), which is important since this is going to run every second in watch mode.
It's ok to leave that this way but we should take note to improve it in another PR.

deps = append(deps, path)
return nil
})
if err != nil {
return nil, errors.Wrap(err, "filepath walk")
}
continue
}
deps = append(deps, l)

deps = append(deps, dep)
}
return deps, nil
}
12 changes: 9 additions & 3 deletions pkg/skaffold/jib/jib_gradle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package jib

import (
"fmt"
"os/exec"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
Expand All @@ -29,11 +30,16 @@ func GetDependenciesGradle(workspace string, a *latest.JibGradleArtifact) ([]str
cmd := getCommandGradle(workspace, a)
deps, err := getDependencies(cmd)
if err != nil {
return nil, errors.Wrapf(err, "getting jib-gradle dependencies")
return nil, errors.Wrapf(err, "getting jibGradle dependencies")
}
return deps, nil
}

func getCommandGradle(workspace string, _ /* a */ *latest.JibGradleArtifact) *exec.Cmd {
return getCommand(workspace, "gradle", getWrapperGradle(), []string{"_jibSkaffoldFiles", "-q"})
func getCommandGradle(workspace string, a *latest.JibGradleArtifact) *exec.Cmd {
args := []string{"_jibSkaffoldFiles", "-q"}
if a.Project != "" {
// multi-module
args[0] = fmt.Sprintf(":%s:%s", a.Project, args[0])
}
return getCommand(workspace, "gradle", getWrapperGradle(), args)
}
34 changes: 31 additions & 3 deletions pkg/skaffold/jib/jib_gradle_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,33 @@ import (
"strings"
"testing"

"fmt"
"path/filepath"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
"github.com/GoogleContainerTools/skaffold/testutil"
"github.com/pkg/errors"
)

func TestGetDependenciesGradle(t *testing.T) {
tmpDir, cleanup := testutil.NewTempDir(t)
defer cleanup()

tmpDir.Write("dep1", "")
tmpDir.Write("dep2", "")

dep1 := filepath.Join(tmpDir.Root(), "dep1")
dep2 := filepath.Join(tmpDir.Root(), "dep2")

var tests = []struct {
description string
stdout string
err error
}{
{
description: "success",
stdout: "dep1\ndep2\n\n\n",
stdout: fmt.Sprintf("%s\n%s\n\n\n", dep1, dep2),
err: nil,
},
{
Expand All @@ -59,9 +71,9 @@ func TestGetDependenciesGradle(t *testing.T) {

deps, err := GetDependenciesGradle(tmpDir.Root(), &latest.JibGradleArtifact{})
if test.err != nil {
testutil.CheckErrorAndDeepEqual(t, true, err, "getting jib-gradle dependencies: "+test.err.Error(), err.Error())
testutil.CheckErrorAndDeepEqual(t, true, err, "getting jibGradle dependencies: "+test.err.Error(), err.Error())
} else {
testutil.CheckDeepEqual(t, []string{"dep1", "dep2"}, deps)
testutil.CheckDeepEqual(t, []string{dep1, dep2}, deps)
}
})
}
Expand All @@ -82,6 +94,14 @@ func TestGetCommandGradle(t *testing.T) {
return getCommand(workspace, "gradle", "ignored", []string{"_jibSkaffoldFiles", "-q"})
},
},
{
description: "gradle default with project",
jibGradleArtifact: latest.JibGradleArtifact{Project: "project"},
filesInWorkspace: []string{},
expectedCmd: func(workspace string) *exec.Cmd {
return getCommand(workspace, "gradle", "ignored", []string{":project:_jibSkaffoldFiles", "-q"})
},
},
{
description: "gradle with wrapper",
jibGradleArtifact: latest.JibGradleArtifact{},
Expand All @@ -90,6 +110,14 @@ func TestGetCommandGradle(t *testing.T) {
return getCommand(workspace, "ignored", getWrapperGradle(), []string{"_jibSkaffoldFiles", "-q"})
},
},
{
description: "gradle with wrapper and project",
jibGradleArtifact: latest.JibGradleArtifact{Project: "project"},
filesInWorkspace: []string{getWrapperGradle()},
expectedCmd: func(workspace string) *exec.Cmd {
return getCommand(workspace, "ignored", getWrapperGradle(), []string{":project:_jibSkaffoldFiles", "-q"})
},
},
}

for _, test := range tests {
Expand Down
2 changes: 1 addition & 1 deletion pkg/skaffold/jib/jib_maven.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import (
func GetDependenciesMaven(workspace string, a *latest.JibMavenArtifact) ([]string, error) {
deps, err := getDependencies(getCommandMaven(workspace, a))
if err != nil {
return nil, errors.Wrapf(err, "getting jib-maven dependencies")
return nil, errors.Wrapf(err, "getting jibMaven dependencies")
}
return deps, nil
}
Expand Down
18 changes: 15 additions & 3 deletions pkg/skaffold/jib/jib_maven_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,33 @@ import (
"strings"
"testing"

"fmt"
"path/filepath"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
"github.com/GoogleContainerTools/skaffold/testutil"
"github.com/pkg/errors"
)

func TestGetDependenciesMaven(t *testing.T) {
tmpDir, cleanup := testutil.NewTempDir(t)
defer cleanup()

tmpDir.Write("dep1", "")
tmpDir.Write("dep2", "")

dep1 := filepath.Join(tmpDir.Root(), "dep1")
dep2 := filepath.Join(tmpDir.Root(), "dep2")

var tests = []struct {
description string
stdout string
err error
}{
{
description: "success",
stdout: "dep1\ndep2\n\n\n",
stdout: fmt.Sprintf("%s\n%s\n\n\n", dep1, dep2),
err: nil,
},
{
Expand All @@ -59,9 +71,9 @@ func TestGetDependenciesMaven(t *testing.T) {

deps, err := GetDependenciesMaven(tmpDir.Root(), &latest.JibMavenArtifact{})
if test.err != nil {
testutil.CheckErrorAndDeepEqual(t, true, err, "getting jib-maven dependencies: "+test.err.Error(), err.Error())
testutil.CheckErrorAndDeepEqual(t, true, err, "getting jibMaven dependencies: "+test.err.Error(), err.Error())
} else {
testutil.CheckDeepEqual(t, []string{"dep1", "dep2"}, deps)
testutil.CheckDeepEqual(t, []string{dep1, dep2}, deps)
}
})
}
Expand Down
43 changes: 37 additions & 6 deletions pkg/skaffold/jib/jib_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,30 @@ import (
"os/exec"
"testing"

"fmt"
"path/filepath"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
"github.com/GoogleContainerTools/skaffold/testutil"
)

func TestGetDependencies(t *testing.T) {
tmpDir, cleanup := testutil.NewTempDir(t)
defer cleanup()

tmpDir.Write("dep1", "")
tmpDir.Write("dep2", "")
tmpDir.Write("dep3/fileA", "")
tmpDir.Write("dep3/sub/path/fileB", "")

dep1 := filepath.Join(tmpDir.Root(), "dep1")
dep2 := filepath.Join(tmpDir.Root(), "dep2")
dep3 := filepath.Join(tmpDir.Root(), "dep3")
dep3FileA := filepath.Join(tmpDir.Root(), "dep3/fileA")
dep3Sub := filepath.Join(tmpDir.Root(), "dep3/sub")
dep3SubPath := filepath.Join(tmpDir.Root(), "dep3/sub/path")
dep3SubPathFileB := filepath.Join(tmpDir.Root(), "dep3/sub/path/fileB")

var tests = []struct {
stdout string
expectedDeps []string
Expand All @@ -34,20 +53,32 @@ func TestGetDependencies(t *testing.T) {
expectedDeps: nil,
},
{
stdout: "deps1\ndeps2",
expectedDeps: []string{"deps1", "deps2"},
stdout: fmt.Sprintf("%s\n%s", dep1, dep2),
expectedDeps: []string{dep1, dep2},
},
{
stdout: "deps1\ndeps2\n",
expectedDeps: []string{"deps1", "deps2"},
stdout: fmt.Sprintf("%s\n%s\n", dep1, dep2),
expectedDeps: []string{dep1, dep2},
},
{
stdout: "\n\n\n",
expectedDeps: nil,
},
{
stdout: "\n\ndeps1\n\ndeps2\n\n\n",
expectedDeps: []string{"deps1", "deps2"},
stdout: fmt.Sprintf("\n\n%s\n\n%s\n\n\n", dep1, dep2),
expectedDeps: []string{dep1, dep2},
},
{
stdout: dep3,
expectedDeps: []string{dep3, dep3FileA, dep3Sub, dep3SubPath, dep3SubPathFileB},
},
{
stdout: fmt.Sprintf("%s\n%s\n%s\n", dep1, dep2, dep3),
expectedDeps: []string{dep1, dep2, dep3, dep3FileA, dep3Sub, dep3SubPath, dep3SubPathFileB},
},
{
stdout: fmt.Sprintf("%s\nnonexistent\n%s\n%s\n", dep1, dep2, dep3),
expectedDeps: []string{dep1, dep2, dep3, dep3FileA, dep3Sub, dep3SubPath, dep3SubPathFileB},
},
}

Expand Down
5 changes: 4 additions & 1 deletion pkg/skaffold/runner/runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,10 @@ func dependenciesForArtifact(a *latest.Artifact) ([]string, error) {

var p []string
for _, path := range paths {
p = append(p, filepath.Join(a.Workspace, path))
if !filepath.IsAbs(path) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be fine in regards to the other artifacts right? There should be no case where a dep would be absolute yet need to resolve against workspace?

path = filepath.Join(a.Workspace, path)
}
p = append(p, path)
}
return p, nil
}