Implements jib.GetDependenciesMaven/GetDependenciesGradle. by coollog · Pull Request #1058 · GoogleContainerTools/skaffold

coollog · 2018-09-27T23:02:57Z

This PR builds off of #1043 and should merge that before merging this.

Implements jib/jib.go with GetDependenciesMaven and GetDependenciesGradle.

For Maven, calls out to mvn jib:_skaffold-files -q. Uses the wrapper mvnw if present. Uses Windows wrapper mvnw.cmd if present and OS is Windows.

For Gradle, calls out to gradle _jibSkaffoldFiles -q. Uses the wrapper gradlew if present. Uses Windows wrapper gradlew.bat if present and OS is Windows.

Note that this only covers the single-module use case and multi-module use case will be added in future PRs.

@GoogleContainerTools/java-tools

coollog · 2018-09-27T23:04:15Z

pkg/skaffold/jib/jib.go

+	return err == nil
+}
+
+func getDependencies(workspace string, buildFile string, defaultExecutable string, windowsExecutable string, isWindows bool, subCommand []string, artifactName string) ([]string, error) {


Would a struct over a long param list be preferred?

Could isWindows be a package variable? I created some helpers for execution that looks for the maven/gradle wrappers with different extensions. (https://github.com/briandealwis/skaffold/tree/jib-build)

Linking the commit for reference: briandealwis@1ec213d

I'll try to change that and see if the testing works well.

Hmm, not sure if package variables are good for readability? @GoogleContainerTools/container-tools

briandealwis · 2018-09-28T15:40:38Z

pkg/skaffold/jib/jib.go

+	if isWindows {
+		if exists(workspace, windowsExecutable) {
+			executable = "call"
+			subCommand = append([]string{windowsExecutable}, subCommand...)


Do we need to use call from within Go?

Ah, I actually need to use cmd.exe /C ... instead. Also, @GoogleContainerTools/container-tools is there a style preference for using back-ticks for string constants vs quotes?

Not that I'm aware of: we generally follow standard Go & Kubernetes Coding Conventions: https://github.com/kubernetes/community/blob/master/contributors/guide/coding-conventions.md

In practice, raw string literals (back-ticks) in Go are only used for strings containing a back-slash or double quotes. Other string declarations should probably use the interpreted string literal form (double quotes) unless there is a special requirement to do so.

For specific discussion of string constants (ones declared with const), see the "String constants" section on https://blog.golang.org/constants

Thanks for clarifying!

briandealwis · 2018-09-28T15:52:13Z

pkg/skaffold/jib/jib.go

+	return getDependencies(workspace, "build.gradle", "gradle", "gradlew.bat", isWindows, []string{"_jibSkaffoldFiles", "-q"}, "jib-gradle")
+}
+
+func exists(workspace string, filename string) bool {


It would be worth making this an isDir() that also tests that the !info.IsDir()?

Not sure what the directory check is for?

Right now this will return true for a directory.

briandealwis · 2018-09-28T15:54:09Z

pkg/skaffold/jib/jib.go

+	return err == nil
+}
+
+func getDependencies(workspace string, buildFile string, defaultExecutable string, windowsExecutable string, isWindows bool, subCommand []string, artifactName string) ([]string, error) {


Could isWindows be a package variable? I created some helpers for execution that looks for the maven/gradle wrappers with different extensions. (https://github.com/briandealwis/skaffold/tree/jib-build)

loosebazooka · 2018-09-28T17:34:07Z

pkg/skaffold/jib/jib.go

+// All paths are absolute.
+// TODO(coollog): Add support for multi-module projects.
+func GetDependenciesMaven(workspace string, _ /*a*/ *v1alpha3.JibMavenArtifact, isWindows bool) ([]string, error) {
+	return getDependencies(workspace, "pom.xml", "mvn", "mvnw.cmd", isWindows, []string{"jib:_skaffold-files", "-q"}, "jib-maven")


This might need to specify the profile in use.

loosebazooka · 2018-09-28T17:34:27Z

pkg/skaffold/schema/v1alpha3/config.go

+type JibMavenArtifact struct {
+	// Only multi-module
+	Module  string `yaml:"module"`
+	Profile string `yaml:"profile"`


Turns out profile is not multimodule only.

balopat

Operating system related code should go to *_GOOS suffixed files instead of explicit runtime.GOOS checks. Unit tests that are testing platform dependent code can be also separated out to run/be built only on the given OS using build tags.

docs: https://golang.org/pkg/go/build/
See this minikube package as an example: https://github.com/kubernetes/minikube/tree/master/pkg/minikube/cluster

coollog · 2018-09-28T18:56:42Z

@balopat Will change into that format.

briandealwis · 2018-09-28T19:07:19Z

pkg/skaffold/jib/jib.go

+	gradleWindowsWrapper = "gradlew.bat"
+)
+
+func getCommandMaven(workspace string, a *v1alpha3.JibMavenArtifact, isWindows bool) (executable string, subCommand []string) {


Can we generalize this and getCommandGradle to take the subcommand as an argument?

getCommand does take subCommand (defaultSubCommand) as an argument

coollog · 2018-09-28T19:35:04Z

Changed to use *_GOOS filename format - LMK if this looks okay.

briandealwis · 2018-09-28T19:57:11Z

pkg/skaffold/jib/jib.go

+	return getDependencies(workspace, "build.gradle", "gradle", "gradlew.bat", isWindows, []string{"_jibSkaffoldFiles", "-q"}, "jib-gradle")
+}
+
+func exists(workspace string, filename string) bool {


Right now this will return true for a directory.

briandealwis · 2018-09-28T20:07:44Z

pkg/skaffold/jib/jib_darwin.go

+package jib
+
+func getWrapper() string {
+	return "gradlew"


This doesn't seem right?

Oh is this missing something?

Oh oops. Hmm, how did this even pass the tests.

briandealwis · 2018-09-28T20:08:14Z

pkg/skaffold/jib/jib_darwin.go

+
+	wrapperExecutable := getWrapper()
+	if exists(workspace, wrapperExecutable) {
+		executable = "./" + wrapperExecutable


I think it's worth making the path absolute just in case the command-execution sets a different working directory

briandealwis · 2018-09-28T20:11:03Z

pkg/skaffold/jib/jib_linux.go

+package jib
+
+func getWrapper() string {
+	return "gradlew"


Doesn't seem right?

briandealwis · 2018-09-28T20:11:13Z

pkg/skaffold/jib/jib_linux.go

+
+	wrapperExecutable := getWrapper()
+	if exists(workspace, wrapperExecutable) {
+		executable = "./" + wrapperExecutable


Ditto returning an absolute path

briandealwis · 2018-09-28T20:16:45Z

pkg/skaffold/jib/jib_darwin.go

+	return "gradlew"
+}
+
+func getCommand(workspace string, defaultExecutable string, defaultSubCommand []string) (executable string, subCommand []string) {


Can you capitalize this so that it can be used elsewhere?

Or provide InvokeMaven and InvokeGradle variants that hide the wrapper bit.

I think I'll leave that to another PR - one that would use this method from another package - it's probably good to keep visibility as low as possible until it needs to be exposed.

I want to replace what I have :-)

Would making getCommandMaven and getCommandGradle exposed be good? I'll change it to include a custom subcommand to use.

pkg/skaffold/jib/jib.go

coollog · 2018-09-28T21:06:23Z

Hmm, it seems if I name a test like jib_non_windows_test.go, it doesn't execute when I do make test on my linux machine. @GoogleContainerTools/container-tools

coollog · 2018-10-02T15:42:11Z

Alright, finally got Windows to pass.

loosebazooka · 2018-10-02T20:48:07Z

pkg/skaffold/jib/jib_linux.go

@@ -0,0 +1,38 @@
+/*


do we really need both jib_linux and jib_darwin? @balopat ?

no just call it jib.go and put // +build linux darwin at the top

you can call it jib_unix.go if you really want

Okay, will go with jib_unix.go since jib.go has common logic shared with jib_windows.go

…n imports ¯\_(ツ)_/¯.

codecov-io · 2018-10-03T15:46:06Z

Codecov Report

Merging #1058 into jib_skaffold will increase coverage by 0.82%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##           jib_skaffold    #1058      +/-   ##
================================================
+ Coverage         40.72%   41.55%   +0.82%     
================================================
  Files                72       77       +5     
  Lines              3104     3148      +44     
================================================
+ Hits               1264     1308      +44     
  Misses             1715     1715              
  Partials            125      125

Impacted Files	Coverage Δ
pkg/skaffold/jib/jib_unix.go	`100% <100%> (ø)`
pkg/skaffold/util/util.go	`51.42% <100%> (+4%)`	⬆️
pkg/skaffold/jib/jib_gradle.go	`100% <100%> (ø)`
pkg/skaffold/jib/jib_gradle_unix.go	`100% <100%> (ø)`
pkg/skaffold/jib/jib_maven.go	`100% <100%> (ø)`
pkg/skaffold/jib/jib_maven_unix.go	`100% <100%> (ø)`
pkg/skaffold/jib/jib.go	`100% <100%> (ø)`	⬆️
... and 2 more

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 3bd47ae...39fb94b. Read the comment docs.

balopat

left some more reorganizational comments and nits

balopat · 2018-10-03T17:23:29Z

pkg/skaffold/jib/jib.go

+
+func getDependencies(workspace string, buildFile string, executable string, subCommand []string, artifactName string) ([]string, error) {
+	if _, err := resolveFile(workspace, buildFile); err != nil {
+		return nil, errors.Errorf("no %s found", buildFile)


nit: use errors.Wrap instead with err

balopat · 2018-10-03T17:30:18Z

pkg/skaffold/jib/jib.go

+}
+
+// resolveFile resolves the absolute path of the file named filename in directory workspace, erroring if it is not a file
+func resolveFile(workspace string, filename string) (string, error) {


this could live in pkg/skaffold/util/util.go

I would rename it to just AbsFile.

workspace -> dir

balopat · 2018-10-03T17:41:08Z

pkg/skaffold/jib/jib_unix.go

+package jib
+
+func getWrapper(defaultExecutable string) string {
+	switch defaultExecutable {


this smells a little bit like we are trying to decide between maven/gradle logic using arbitrary consts (lack of enums I guess?), while in other cases we are using "Maven/Gradle" postfix. It's starting to get confusing what logic belongs to maven/gradle and what's common. I'd like to see if introducing the subpackages under jib/maven, jib/gradle would help or not.

coollog · 2018-10-03T19:02:45Z

Rearranged the structure into separate jib_maven.go/jib_gradle.go and their respective OS-specific code. Let me know if this structure looks okay and I'll migrate the tests over as well.

balopat · 2018-10-03T19:45:31Z

It looks good to me, go ahead with the tests!

loosebazooka · 2018-10-03T20:04:35Z

pkg/skaffold/jib/jib_unix.go

+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
+)
+
+func getCommand(workspace string, defaultExecutable string, wrapperExecutable string, defaultSubCommand []string) *exec.Cmd {


if there's no alternative to the subCommand, do we need to specify default?

Nope, I'll rename to just subCommand

loosebazooka · 2018-10-03T21:14:33Z

We don't need to cover expanding directories in this pr, right?

coollog · 2018-10-03T21:22:39Z

Tests are updated as well and added TODO for expanding directories in follow-up PR.

balopat

LGTM

coollog requested review from balopat, dgageot, nkubala and r2d4 as code owners September 27, 2018 23:02

coollog commented Sep 27, 2018

View reviewed changes

briandealwis reviewed Sep 28, 2018

View reviewed changes

coollog changed the base branch from master to jib_skaffold September 28, 2018 17:32

loosebazooka reviewed Sep 28, 2018

View reviewed changes

loosebazooka and others added 6 commits September 28, 2018 13:39

Simple use of ctx for linter

f103724

temporarily ignore unused params

ee3d87a

Hopefully now linter is a-ok

6ef3262

Implements jib.GetDependenciesMaven.

16c8731

Adds jib.GetDependenciesGradle.

34307e9

Adds comment.

6895843

coollog force-pushed the jib-GetDependenciesMaven branch from 125e03b to 6895843 Compare September 28, 2018 17:42

balopat suggested changes Sep 28, 2018

View reviewed changes

Reorganize a bit

6a6306e

briandealwis reviewed Sep 28, 2018

View reviewed changes

Changes to use _GOOS filename format.

f01d445

Uses nil instead of empty slice.

78fad7c

briandealwis reviewed Sep 28, 2018

View reviewed changes

pkg/skaffold/jib/jib.go Outdated Show resolved Hide resolved

feedback

71d74dc

coollog added 2 commits September 28, 2018 17:11

fixes

fc3c7ca

Fixes tests.

4e4615f

balopat added the build/jib label Oct 2, 2018

coollog added 2 commits October 2, 2018 11:23

Fixing windows

75f6d17

Fixing windows

8b12a3d

loosebazooka reviewed Oct 2, 2018

View reviewed changes

coollog added 6 commits October 3, 2018 10:31

Removes jib_darwin.go, renames jib_linux.go to jib_unix.go.

073c1d5

Gradle multimodule is already supported.

72c9c10

Adds build top.

585e75b

Apparently I need to have new line between github imports and built-i…

631ce34

…n imports ¯\_(ツ)_/¯.

Reordering imports to see if it fixes the failure.

e895265

Is windows test import format good?

e209291

balopat suggested changes Oct 3, 2018

View reviewed changes

Rearranges structure into separate jib_maven and jib_gradle.

538c4a8

balopat mentioned this pull request Oct 3, 2018

Add support for building projects using jib #1073

Merged

loosebazooka reviewed Oct 3, 2018

View reviewed changes

coollog added 4 commits October 3, 2018 17:05

Migrates tests.

c324f23

Fixes windows test.

f26fc4f

cleanup

67567cc

windows

ac5d025

coollog added 2 commits October 3, 2018 17:18

Adds TODO for expanding directories.

964efd8

Fixes linter.

6416f96

lint

39fb94b

balopat approved these changes Oct 3, 2018

View reviewed changes

balopat merged commit 412101b into GoogleContainerTools:jib_skaffold Oct 3, 2018

coollog deleted the jib-GetDependenciesMaven branch October 3, 2018 22:02

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tstromberg Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balopat left a comment

Choose a reason for hiding this comment

Uh oh!

coollog commented Sep 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coollog commented Sep 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coollog commented Sep 28, 2018

Uh oh!

coollog commented Oct 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coollog commented Sep 27, 2018 •

edited

Loading

tstromberg Sep 28, 2018 •

edited

Loading

codecov-io commented Oct 3, 2018 •

edited

Loading

loosebazooka Oct 3, 2018 •

edited

Loading