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.

Sign up for GitHub

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

Jump to bottom

Resolve input files more comprehensively on jibBuildTar #1754

Merged
merged 2 commits into from
Jun 3, 2019
Merged

Resolve input files more comprehensively on jibBuildTar #1754

merged 2 commits into from
Jun 3, 2019

Conversation

loosebazooka
Copy link
Member

@loosebazooka loosebazooka commented May 31, 2019 •
edited by chanseokoh
Loading

Fixes #1757.

Bug: #1745 (comment)

We haven't been including the all the input to the task correct (this only matter with jibBuildTar which has an @Output defined - all other jib tasks run always).

Also we had some weird code (BuildTarTask:83) that is cleaned up here.

@loosebazooka loosebazooka requested a review from a team May 31, 2019 18:08
briandealwis
briandealwis reviewed May 31, 2019
View reviewed changes
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.

Why do we treat BuildDockerTask and BuildImageTask differently: why no @Input nor @Output? Because they don't have a formal output?

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildTarTask.java
@@ -80,11 +80,7 @@ public void setTargetImage(String targetImage) {
public FileCollection getInputFiles() {
Copy link
Member

Choose a reason for hiding this comment

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

The javadoc makes it sound like only the classes dependencies matter, and there's no mention of the extras.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed.

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/GradleProjectProperties.java Outdated
* @param project the gradle project
* @param extraDirectories the image's configured extra directories
* @return the input files to the task are all the output files for all the dependencies of the
* {@code classes} task
Copy link
Member

Choose a reason for hiding this comment

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

No mention of the extras?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed.

@loosebazooka
Copy link
Member Author

Why do we treat BuildDockerTask and BuildImageTask differently: why no @Input nor @Output? Because they don't have a formal output?

Yes in gradle, unless you have an Output defined, it does not check if things are up to date or not. I feel like BuildDockerTask should also have an output so we don't rebuild a whole image when we don't need to (Is that how it works?)?

@chanseokoh
Copy link
Member

it does not check if things are up to date or not. I feel like BuildDockerTask should also have an output so we don't rebuild a whole image

Isn't it basically that we should always push an image to a Docker daemon regardless of whether there is a change to input? Because often you do docker rmi to remove images and you expect jib:dockerBuild to push an image again if it's missing. I think performance is not a big deal, since we cache base and application layers.

@loosebazooka
Copy link
Member Author

Because often you do docker rmi to remove images and you expect jib:dockerBuild to push an image again if it's missing.

This is a good point, I didn't think of it.

@loosebazooka loosebazooka requested a review from a team June 3, 2019 14:11
@TadCordle
Copy link
Contributor

Will this fix #1757?

@loosebazooka loosebazooka mentioned this pull request Jun 3, 2019
Open
@loosebazooka
Copy link
Member Author

@TadCordle It should fix it.

@chanseokoh chanseokoh mentioned this pull request Jun 3, 2019
Closed
@loosebazooka loosebazooka merged commit 5dba0af into master Jun 3, 2019
@chanseokoh chanseokoh added this to the v1.3.0 milestone Jun 7, 2019
@chanseokoh chanseokoh deleted the fixUpToDateBuildTar branch June 10, 2019 14:46
@chanseokoh chanseokoh mentioned this pull request Jun 10, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@briandealwis briandealwis briandealwis left review comments

@chanseokoh chanseokoh chanseokoh approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
v1.3.0
Development

Successfully merging this pull request may close these issues.

Broken up-to-date checking for BuildTarTask
6 participants
@loosebazooka @chanseokoh @TadCordle @briandealwis @googlebot @kokoro-team