Lists files recursively in jib.getDependencies and other fixes. by coollog · Pull Request #1097 · GoogleContainerTools/skaffold

coollog · 2018-10-04T18:05:21Z

Resolves issues in #1096:

Rename error message to jibMaven and jibGradle.
GetDependenciesGradle process JibGradleArtifact.Project
runner.dependenciesForArtifact seems to try to resolve absolute deps against workspace
GetDependencies* to list files recursively
Error if trying to use Maven multi-module in jib.GetDependenciesMaven

googlebot · 2018-10-04T18:05:29Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

coollog · 2018-10-04T18:08:05Z

/cc @GoogleContainerTools/java-tools

loosebazooka · 2018-10-04T18:17:45Z

~~should we be rebased against that feature branch?~~ ah never mind.

codecov-io · 2018-10-04T19:17:11Z

Codecov Report

Merging #1097 into jib_skaffold will increase coverage by 0.14%.
The diff coverage is 72.41%.

@@               Coverage Diff                @@
##           jib_skaffold    #1097      +/-   ##
================================================
+ Coverage         43.37%   43.51%   +0.14%     
================================================
  Files                78       78              
  Lines              3341     3362      +21     
================================================
+ Hits               1449     1463      +14     
- Misses             1759     1764       +5     
- Partials            133      135       +2

Impacted Files	Coverage Δ
pkg/skaffold/runner/runner.go	`51.78% <0%> (-0.47%)`	⬇️
pkg/skaffold/jib/jib_gradle.go	`100% <100%> (ø)`	⬆️
pkg/skaffold/jib/jib_maven.go	`83.33% <33.33%> (-16.67%)`	⬇️
pkg/skaffold/jib/jib.go	`88% <82.35%> (-12%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 780b26d...30ce7ed. Read the comment docs.

googlebot · 2018-10-04T19:24:14Z

CLAs look good, thanks!

coollog · 2018-10-04T19:24:32Z

Merged #1073 .

coollog · 2018-10-04T19:26:48Z

pkg/skaffold/jib/jib.go

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


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?

coollog · 2018-10-04T19:28:53Z

pkg/skaffold/runner/runner.go

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


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?

loosebazooka · 2018-10-05T19:26:37Z

Logically looks fine to me, would probably want to have someone on @GoogleContainerTools/container-tools check out the go code.

dgageot

I’ll have a look tomorrow

dgageot · 2018-10-08T05:03:19Z

Sorry, this needs to be rebased

googlebot · 2018-10-08T14:06:22Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

googlebot · 2018-10-08T14:06:56Z

CLAs look good, thanks!

coollog · 2018-10-08T14:07:13Z

This is rebased and good for review.

dgageot · 2018-10-08T14:12:37Z

pkg/skaffold/jib/jib.go

+			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 {


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.

Lists files recursively in jib.getDependencies.

a5775f1

coollog requested review from balopat, dgageot, nkubala and r2d4 as code owners October 4, 2018 18:05

coollog mentioned this pull request Oct 4, 2018

Resolve issues to finish Jib builder. #1096

Closed

11 tasks

coollog changed the base branch from master to jib_skaffold October 4, 2018 18:06

goimports

a6f4ddc

Account for JibGradleArtifact.Project.

d566548

coollog changed the title ~~Lists files recursively in jib.getDependencies.~~ Lists files recursively in jib.getDependencies and other fixes. Oct 4, 2018

coollog added 2 commits October 4, 2018 15:20

goimports

16007c3

Merge branch 'jib_skaffold' into jib-walk-deps

396fbb2

coollog commented Oct 4, 2018

View reviewed changes

Removes debug message.

5374868

coollog commented Oct 4, 2018

View reviewed changes

Errors if trying to use Maven multi-module.

1e643fc

dgageot reviewed Oct 5, 2018

View reviewed changes

Merge branch 'jib-unified' into jib-walk-deps

d0c069d

Merge branch 'jib_skaffold' into jib-walk-deps

30ce7ed

dgageot approved these changes Oct 8, 2018

View reviewed changes

dgageot merged commit 751476e into GoogleContainerTools:jib_skaffold Oct 8, 2018

loosebazooka mentioned this pull request Oct 8, 2018

Merge master into jib_skaffold feature branch #1116

Merged

coollog deleted the jib-walk-deps branch October 8, 2018 18:58

coollog mentioned this pull request Oct 8, 2018

Improves jib.getDependencies. #1125

Merged

Conversation

coollog commented Oct 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Oct 4, 2018

Uh oh!

coollog commented Oct 4, 2018

Uh oh!

loosebazooka commented Oct 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Oct 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

googlebot commented Oct 4, 2018

Uh oh!

coollog commented Oct 4, 2018

Uh oh!

coollog Oct 4, 2018

Choose a reason for hiding this comment

Uh oh!

coollog Oct 4, 2018

Choose a reason for hiding this comment

Uh oh!

loosebazooka commented Oct 5, 2018

Uh oh!

dgageot left a comment

Choose a reason for hiding this comment

Uh oh!

dgageot commented Oct 8, 2018

Uh oh!

googlebot commented Oct 8, 2018

Uh oh!

googlebot commented Oct 8, 2018

Uh oh!

coollog commented Oct 8, 2018

Uh oh!

dgageot Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coollog commented Oct 4, 2018 •

edited

Loading

loosebazooka commented Oct 4, 2018 •

edited

Loading

codecov-io commented Oct 4, 2018 •

edited

Loading