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

Allow setting file timestamps from plugins #1818

Merged
merged 23 commits into from
Jul 30, 2019
Merged

Allow setting file timestamps from plugins #1818

merged 23 commits into from
Jul 30, 2019

Conversation

chanseokoh
Copy link
Member

@chanseokoh chanseokoh commented Jun 28, 2019 •
edited
Loading

Migrated from #1782. Contains commits from an external contributor, so CLA bot keeps complaining. Manaully marking CLA OK.

Fixes #1608.

// ISO 8601 date time format
container.filesModificationTime = '2011-12-03T10:15:30+09:00'
container.filesModificationTime = 'EPOCH_PLUS_SECOND` // default
<container>
  <filesModificationTime>2011-12-03T10:15:30Z</filesModificationTime>
</container>

I noticed the target file modification time should be factored in when determining if an application layer for the files were cached, so I modified LayerEntriesSelector accordingly.

@warrkan @chanseokoh
Allow setting file timestamps from plugins (#1608) (#1782)
99973d3 
* Allow setting file timestamps from plugins (#1608)
* Addressed comment related to unnecessary extraDirectories modificationTimes configuration
* Addressed remaining comments
@googlebot

This comment has been minimized.

Sign in to view
@googlebot

This comment has been minimized.

Sign in to view
@googlebot googlebot removed the cla: no label Jun 28, 2019
@googlebot googlebot added cla: no and removed cla: yes labels Jul 2, 2019
@GoogleContainerTools GoogleContainerTools deleted a comment from googlebot Jul 2, 2019
@GoogleContainerTools GoogleContainerTools deleted a comment from googlebot Jul 2, 2019
@googlebot googlebot added cla: no and removed cla: yes labels Jul 2, 2019
@GoogleContainerTools GoogleContainerTools deleted a comment from googlebot Jul 2, 2019
@chanseokoh
Copy link
Member Author

Restored the changes to LayerEntriesSelector to factor in target modification time when computing a hash. Sorry for going back and forth. This should be the final stable state.

@chanseokoh chanseokoh requested a review from a team July 29, 2019 20:13
TadCordle
TadCordle reviewed Jul 29, 2019
View reviewed changes
Copy link
Contributor

@TadCordle TadCordle left a comment

Choose a reason for hiding this comment

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

Mostly looks good, just some small changes and observations.

jib-core/src/main/java/com/google/cloud/tools/jib/api/JavaContainerBuilder.java Show resolved Hide resolved
jib-core/src/main/java/com/google/cloud/tools/jib/image/ReproducibleLayerBuilder.java Outdated Show resolved Hide resolved
...rc/integration-test/java/com/google/cloud/tools/jib/gradle/SingleProjectIntegrationTest.java
@@ -135,7 +135,8 @@ private static void assertExtraDirectoryDeprecationWarning(String pomXml)
BuildResult buildResult =
JibRunHelper.buildToDockerDaemon(simpleTestProject, targetImage, pomXml);
Assert.assertEquals(
"Hello, world. \nrw-r--r--\nrw-r--r--\nfoo\ncat\n",
"Hello, world. \n1970-01-01T00:00:01Z\nrw-r--r--\nrw-r--r--\nfoo\ncat\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

The output for this test program is starting to get more and more complicated, I wonder if we should think about making it more readable (not this PR)

Copy link
Member Author

Choose a reason for hiding this comment

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

I felt the same. It's growing duplicates unnecessarily.

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildDockerTask.java Show resolved Hide resolved
jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildImageTask.java Show resolved Hide resolved
jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildTarTask.java Show resolved Hide resolved
@chanseokoh chanseokoh merged commit 62c9cab into master Jul 30, 2019
@chanseokoh chanseokoh deleted the i1608-warrkan-file-timestamps branch July 30, 2019 16:50
chanseokoh added a commit that referenced this pull request Jul 31, 2019
@chanseokoh
Update CHANGELOG for #1818 and #1840 (#1865)
8d82956
@chanseokoh chanseokoh mentioned this pull request Aug 5, 2019
Closed
chanseokoh added a commit that referenced this pull request Aug 16, 2019
@chanseokoh
Update README and FAQ for #1818, #1814, and #1888 (#1866)
a177e3e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loosebazooka loosebazooka loosebazooka left review comments

@briandealwis briandealwis briandealwis left review comments

@TadCordle TadCordle TadCordle approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow setting file timestamps from plugins
6 participants
@chanseokoh @googlebot @loosebazooka @briandealwis @TadCordle @warrkan