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

Update gradle extraDirectory config parameter to include path and permissions field #1178

Merged
merged 7 commits into from
Oct 23, 2018

Conversation

TadCordle
Copy link
Contributor

@TadCordle TadCordle commented Oct 22, 2018

Toward #794

extraDirectory configuration parameter is now an object with a path and permissions field, instead of just being a file

  • extraDirectory.path covers the original purpose of extraDirectory
  • extraDirectory.permissions is a map from path on container to an octal string representation of the file's permission bits

For example, if your extra directory is (project dir)/src/main/custom-jib, and you want to give executable permissions to path/to/file1 and path/to/file2 within, you would add this configuration to your build.gradle:

jib {
  ...
  extraDirectory {
    path = file('src/main/custom-jib')
    permissions = [
      '/path/to/file1': '755',
      '/path/to/file2': '755'
    ]
  }
}

The old method of configuring the extra directory is also still supported:

jib {
  ...
  extraDirectory = file('src/main/custom-jib')
}

@chanseokoh
Copy link
Member

I wonder how this will look like in Maven, because currently for other maps, an HTML tag name works as a key to the maps. But obviously we can't do </path/to/file2>777<//path/to/file2>.

Assuming that this will break the existing users who configured extraDirectory, another thing I'd like to make sure is that existing users will get a good error message so that they have no friction in updating their config, not like a native Java type mismatch error or such.

@TadCordle
Copy link
Contributor Author

I wonder how this will look like in Maven, because currently for other maps, an HTML tag name works as a key to the maps. But obviously we can't do </path/to/file2>777<//path/to/file2>.

Judging by the original comment, I think we can make the map an alternating list style sort of thing:

<extraDirectory>
  <path>extra/dir/path</path>
  <permissions>
    <file>/path/file1</file>
    <mode>755</mode>
    <file>/path/file2</file>
    <mode>755</mode>
  </permissions>
</extraDirectory>

Assuming that this will break the existing users who configured extraDirectory, another thing I'd like to make sure is that existing users will get a good error message so that they have no friction in updating their config, not like a native Java type mismatch error or such.

Yeah, I think I would have liked to deprecate the parameter first, but considering we're keeping the parameter and just changing its type I wasn't sure how the best way to do this.

@coollog
Copy link
Contributor

coollog commented Oct 23, 2018

A slight correction on the Maven config, but otherwise looks good:

<extraDirectory>
  <path>extra/dir/path</path>
  <permissions>
    <permission>
      <file>/path/file1</file>
      <mode>755</mode>
    </permission>
    <permission>
      <file>/path/file2</file>
      <mode>755</mode>
    </permission>
  </permissions>
</extraDirectory>

I think for Gradle, we could actually support both the use of extraDirectory as a closure with the path and permissions fields or as a file to just be the path.
I believe in Maven, there's some way to make it so that if the user just uses <extraDirectory>some/path</extraDirectory>, it can take some/path as the path nested input, but I'll need to find that again.

@TadCordle
Copy link
Contributor Author

TadCordle commented Oct 23, 2018

Hmmm I just noticed that changing permissions in the build file without changing anything in the extra directory itself causes the extra layer to not be updated because file permissions aren't cached.

Edit: still works with ./gradlew clean, so maybe I should just make an issue and fix it in another PR.

@TadCordle
Copy link
Contributor Author

Have updated to support both jib.extraDirectory { ... } and jib.extraDirectory = file(...).

@coollog
Copy link
Contributor

coollog commented Oct 23, 2018

Good catch, we'll need to update the selector to include permissions then.

Copy link
Contributor

@coollog coollog left a comment

Choose a reason for hiding this comment

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

LGTM, will have someone else do the final approval.

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.

LGTM. I'll just do an actual test real quick.

@@ -184,5 +187,23 @@ public static ImageReference getGeneratedTargetDockerTag(
return ImmutableList.copyOf(items);
}

/**
* Validates and converts a {@code String->String} map to an {@code
Copy link
Member

Choose a reason for hiding this comment

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

Let's mention that the input map is a file-path-to-file-permission map. Without any context, it's not clear what the map is about.

* @param extraDirectory path to the source directory for the extra files layer
* @param permissions map from path on container to file permissions
Copy link
Member

Choose a reason for hiding this comment

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

Let's mention this applies only to the files from extraDirectory.

@TadCordle TadCordle merged commit 90a0b66 into master Oct 23, 2018
@TadCordle TadCordle deleted the file-permissions-gradle branch October 23, 2018 20:09
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.

4 participants