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.

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

Skaffold init support for polyglot-maven projects #4871

Merged
merged 2 commits into from
Oct 8, 2020

Conversation

MarlonGamez
Copy link
Contributor

Fixes #4335

Adds support for skaffold init to detect jib as a builder in different pom file extensions that are supported by https://github.com/takari/polyglot-maven.

I've added tests for a couple of extensions, but wasn't sure if I should for each case because I didn't want to clutter the tests with what is essentially just testing string matching

@google-cla google-cla bot added the cla: yes label Oct 7, 2020
@MarlonGamez MarlonGamez changed the title Skaffold init support for polyglot-maven Skaffold init support for polyglot-maven projects Oct 7, 2020
@codecov
Copy link

codecov bot commented Oct 7, 2020

Codecov Report

Merging #4871 into master will decrease coverage by 0.03%.
The diff coverage is 62.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4871      +/-   ##
==========================================
- Coverage   71.88%   71.84%   -0.04%     
==========================================
  Files         356      356              
  Lines       12218    12233      +15     
==========================================
+ Hits         8783     8789       +6     
- Misses       2785     2792       +7     
- Partials      650      652       +2     
Impacted Files Coverage Δ
pkg/skaffold/build/jib/init.go 81.15% <62.50%> (-5.88%) ⬇️
pkg/skaffold/docker/image.go 79.34% <0.00%> (-1.41%) ⬇️

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 ff33ac0...97b5bf8. Read the comment docs.

builderType = JibMaven
executable = "mvn"
wrapper = "mvnw"
searchString = "<artifactId>jib-maven-plugin</artifactId>"
taskName = "jib:_skaffold-init"
consoleFlag = "--batch-mode"

// Polyglot maven support: support pom files of different extension types
Copy link
Member

Choose a reason for hiding this comment

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

There's an easier and more robust way: polyglot maven requires a .mvn/extensions.xml file. Polyglot supports a bunch of other languages, including yaml 🙀

@@ -89,13 +89,30 @@ func validate(path string, enableGradleAnalysis bool) []ArtifactConfig {
var builderType PluginType
var executable, wrapper, taskName, searchString, consoleFlag string
switch {
case strings.HasSuffix(path, "pom.xml"):
case isPomFile(path):
builderType = JibMaven
executable = "mvn"
wrapper = "mvnw"
searchString = "<artifactId>jib-maven-plugin</artifactId>"
Copy link
Member

Choose a reason for hiding this comment

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

TBH I think just searching for jib-maven-plugin is enough. After all, this string would be fooled if someone had jib-maven-plugin as a <dependency>.

@MarlonGamez
Copy link
Contributor Author

@briandealwis So do you think anything needs to be done beyond checking if something's a pom.* file and then searching for "jib-maven-plugin"?

Copy link
Member

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

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

I was going to say that we should check for the existence of .mvn/extensions.xml, but it could exist further up in the tree. The chance of a pom.* that just happens to have the string jib-maven-plugin without being a Jib project is pretty slim.

@MarlonGamez MarlonGamez merged commit d7e1821 into GoogleContainerTools:master Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

skaffold init should detect Polyglot Maven projects
2 participants