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 methods for setting file timestamps #1613

Merged
merged 19 commits into from
Apr 12, 2019
Merged

Conversation

TadCordle
Copy link
Contributor

@TadCordle TadCordle commented Apr 9, 2019

Adds overloads for LayerConfiguration.Builder#addEntryRecursive that take providers for file permissions/file modification timestamps to apply permissions and timestamps on a per-file basis.

Fixes #1607.

Copy link
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.

This looks good to me.

One thing for a thought: previously the file timestamp was part of LayerConfiguration, which makes sense as it is a property of a file like permissions which is still in LayerConfiguration. Now the timestamp provider is set in BuildConfiguration. I think this is fine, but I wonder if others have different opinions.

@TadCordle TadCordle marked this pull request as ready for review April 10, 2019 16:45
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.

Thanks for prototyping this. It seems to a bit more complicated than I imagined. Here are my thoughts:

  • I think we should probably make the file-permissions configurable too (so the below should extend to FilePermissions)
  • I think we want to use a provider rather than single value, so Function<Path,Instant>; we should probably have this on the Jib class so that it's easy to reference (class Jib { static Function<Path,Long> defaultFileModificationTimeProvider = path -> 1000; })
    • if someone wants to use real timestamps set to current then they can just use p -> Files.getLastModifiedTime(p)
  • LayerConfiguration.Builder would require a file modification time provider to be used for subsequent calls to addEntry without an explicit file modification time
  • LayerConfiguration.builder() would use the our default epoch+1s provider
  • JibContainerBuilder and JavaContainerBuilder would take and cascade a provider down — so LayerConfiguration.builder() would just be a convenience for Jib Core users, whereas our plugins should always explicitly pass down the provider

@chanseokoh
Copy link
Member

I think we want to use a provider rather than single value, so Function<Path,Instant>

The PR uses a provider (effectively Function<AbsoluteUnixPath, Instant), if you haven't noticed.

@briandealwis
Copy link
Member

Weird — I must have somehow looking from an earlier commit. This current state looks way better. But I think we should use this provider to provide defaults which can still be explicitly over-ridden in addEntry(...).

@TadCordle
Copy link
Contributor Author

TadCordle commented Apr 10, 2019

If we do allow setting a timestamp provider on a per-LayerEntry basis, I wonder if we even need the AbsoluteUnixPath argument in the provider anymore?

Edit: Actually, source path might be more useful for something like git commit history / getting the actual mod time of the file.

@TadCordle
Copy link
Contributor Author

TadCordle commented Apr 11, 2019

Sorry to keep changing this so much. Trying this now:

  • addEntry may take a single FilePermissions and Instant
  • addEntryRecursive may take a Function BiFunction that determines the permissions/timestamp on a per-file basis
  • Also moved the default FilePermissions stuff out of LayerEntry, which meant changing the constructor, which meant changing a bunch of tests.

Copy link
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.

Generally looks good to me.

@TadCordle TadCordle merged commit 4976cb0 into master Apr 12, 2019
@TadCordle TadCordle deleted the i1079-configurable-timestamp branch April 12, 2019 20:01
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.

Add methods to jib-core to allow setting timestamps of files on container
5 participants