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

Add gradle/maven sync parts + restructure tests #3474

Merged
merged 3 commits into from
Jan 7, 2020

Conversation

loosebazooka
Copy link
Member

Towards #2901

Adds in functions to get the correct sync command. Refactor out some code to share the generation of build args in maven/gradle.

This PR also heavily restructures the jib maven/gradle tests to isolate testing to the methods under test while offering some rudimentary "mockito style" verify to ensure parameters are passed though.

@codecov
Copy link

codecov bot commented Jan 3, 2020

Codecov Report

Merging #3474 into master will decrease coverage by <.01%.
The diff coverage is 91.3%.

Impacted Files Coverage Δ
pkg/skaffold/build/jib/jib.go 72.38% <0%> (-1.1%) ⬇️
pkg/skaffold/build/jib/gradle.go 100% <100%> (ø) ⬆️
pkg/skaffold/build/jib/maven.go 100% <100%> (ø) ⬆️
...affold/kubernetes/portforward/kubectl_forwarder.go 65.85% <0%> (-2.44%) ⬇️

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.

LGTM. Two things I thought of when looking through:

  1. Having xxxArgs() and xxxArgsFunc() seems a bit error-prone.
  2. Would it make sense to rename Generate{Maven|Gradle}Args to Generate{Maven|Gradle}BuildArgs? It seemed a bit odd to see them call mavenBuildArgsFunc

@loosebazooka
Copy link
Member Author

Yeah, I think my naming is pretty poor on those methods, I'll go look at renaming it.

@loosebazooka
Copy link
Member Author

@briandealwis Actually touching those names makes us also touch the gcb package. I'll do it in a followup.

@loosebazooka loosebazooka merged commit a84596b into master Jan 7, 2020
@loosebazooka loosebazooka deleted the jib-sync-parts-1 branch January 7, 2020 15:41
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.

4 participants