Skip to content

Use correct configuration for Skaffold dependencies#2188

Merged
chanseokoh merged 1 commit intoGoogleContainerTools:masterfrom
lightoze:patch-1
Dec 5, 2019
Merged

Use correct configuration for Skaffold dependencies#2188
chanseokoh merged 1 commit intoGoogleContainerTools:masterfrom
lightoze:patch-1

Conversation

@lightoze
Copy link
Copy Markdown
Contributor

@lightoze lightoze commented Dec 5, 2019

Because configurations were changed in Gradle 5, existing implementation fails to detect dependencies between project modules.

@googlebot

This comment has been minimized.

@lightoze
Copy link
Copy Markdown
Contributor Author

lightoze commented Dec 5, 2019

@googlebot I signed it!

@googlebot

This comment has been minimized.

@googlebot googlebot added cla: yes and removed cla: no labels Dec 5, 2019
Copy link
Copy Markdown
Member

@chanseokoh chanseokoh left a comment

Choose a reason for hiding this comment

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

Awesome! Very cool to see you dug into this far.

currentProject.getConfigurations().getByName("runtime").getHierarchy()) {
currentProject
.getConfigurations()
.getByName(JavaPlugin.RUNTIME_CLASSPATH_CONFIGURATION_NAME)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@lightoze in this class, there's another use of getByName("runtime") for SNAPSHOT, non-project dependencies. Does it need to be changed as well?

    for (File file : project.getConfigurations().getByName("runtime")) {
      if (!projectDependencyJars.contains(file) && file.toString().contains("SNAPSHOT")) {

And you said this is broken with Gradle 5. After changing this, will it continue to work with Gradle 4.9? (Gradle 4.9 is the minimum version Jib supports/requires.)

@loosebazooka looks like the value is now changed to "runtimeClasspath" from "runtime." Does this make sense?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@chanseokoh probably that should be changed too, but it does not affect the project I have on hands so I have nothing to test on. As such "runtime" is deprecated and should be used if only for compatibility.
Gradle 4 should continue to work but I haven't tested.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah I think with 2.0 we plan on going to minimum of gradle 5 anyway: #2140

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also, yeah I agree we should use runtimeClasspath everywhere...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@chanseokoh @loosebazooka OK changed that too and reworked patch into a single commit.

Copy link
Copy Markdown
Member

@chanseokoh chanseokoh left a comment

Choose a reason for hiding this comment

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

Great! Thanks for fixing this.

@chanseokoh
Copy link
Copy Markdown
Member

I'll temporarily override the kokoro-macos failure (due to #2189) and merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants