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

Proposal RFC: Java Agents and More Layers #455

Merged
merged 11 commits into from Aug 23, 2018
Merged

Conversation

coollog
Copy link
Contributor

@coollog coollog commented Jun 27, 2018

Update: This proposal will probably be retired in favor of the incubating src/main/jib convention. (#489)

Request for comments. Approval upon merge.

Preview at https://github.com/GoogleContainerTools/jib/blob/more-layers-proposal/proposals/java_agents_and_more_layers.md

@linux-china
@saturnism

Fixes #378
Fixes #213 (closes in favor of this)
Regards #403

addFile 'another/file', '/another/path/on/image'
```

All of the files would be added to a single new layer.
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to specify this implementation detail? It might make sense to place larger file trees in separate layers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Placing in separate layers may be a lot more complicated, since we would need some sort of decider that would deterministically place the files into a certain number of layers. I can remove this as a detail though since it is implementation-level.


The configuration for adding additional files (including Java agents) would look something like:

*(The exact configuration is to be decided before this PR is merged. Alternative naming for this parameter include: `additionalFiles`, `extraFiles`, and `copyFiles`.)*
Copy link
Member

Choose a reason for hiding this comment

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

Reading these examples, the singular addFile implies that each item is for a single file, and that I'd need to list out each file to copy to a directory. I thought we would allow specifying a directory to have the directory tree to be copied?

In which case we also need to specify the semantics for directories. Perhaps should adopt the rsync source-path semantics?

If the from location is a directory and has a trailing slash then the contents of that directory are copied. Otherwise the contents are copied into a new directory named for the last component of the from location. All intermediate directories are created as required. The destination specifies a directory in the image. The copy fails if the destination is a file.
For example, given source files config/ssh/ssh_config and config/ssh/sshd_config and wish to copy them into /etc/ssh so that the image contains /etc/ssh/ssh_config and /etc/ssh/sshd_config then we can do either of the following:

  • <from>config/ssh</from><to>/etc</to>
  • <from>config/ssh/</from><to>/etc/ssh</to>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea that sounds like a good way to do it! I'll update this with these semantics for directories. In terms of naming for addFile I was thinking of File as referring to both a directory and a regular file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I was just thinking and maybe we should have it be the semantics of Dockerfile COPY, where essentially:

  • if from is a directory, only the contents are copied, and not the directory itself
  • if to ends with a trailing slash, it is considered a directory. Otherwise, it is considered a regular file.
  • if from refers to multiple files (like the contents of a directory), to must end with a trailing slash, and the from files will be added to that directory.

We won't support glob matching though, at least not until someone says they need it.

Copy link
Member

Choose a reason for hiding this comment

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

Using Docker's COPY semantics makes sense.

<from>another/file</from>
<to>/another/path/on/image</to>
</addFile>
</addFiles>
Copy link
Member

Choose a reason for hiding this comment

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

I tried playing with some examples, and this felt a bit more concise?

<files>
  <copy from="path/to/file" to="/path/in/image"/>
  <copy from="path/to/file" to="/path/in/image"/>
</files>

and with groovy

copyIn 'path/to/file', '/path/on/image'

Using copy avoids any confusion with Dockerfile's ADD and its unpacking? One downside to both is that Docker creates a separate layer for each COPY and ADD, which we don't do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think Maven configuration can support duplicate tags or tag parameters? @loosebazooka ?

Copy link
Member

Choose a reason for hiding this comment

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

Oh jeepers 🤦‍♂️ There does seem to be a way to support relatively arbitrary XML but it's not pretty and I don't think it's warranted by this situation.

Copy link
Member

Choose a reason for hiding this comment

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

yeah that might be a little un-maven-y


### Alternative (Rejected) Proposal

The alternative proposal was rejected because we deemed that layering should be an implementation detail that should not be exposed to the user.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe call this out with **The alternative proposal was rejected …**?

@@ -58,7 +58,7 @@ addFile 'path/to/file', '/path/on/image'
addFile 'another/file', '/another/path/on/image'
```

All of the files would be added to a single new layer.
*Directories will be handled using [rsync](https://linux.die.net/man/1/rsync) source-path semantics, where, for example, `addFile 'path/to/directory', '/path/on/image'` would put all the files in `path/to/directory` into the directory `/path/on/image/directory`, but `addFile 'path/to/directory', '/path/on/image'` (trailing slash) would put all the files into the directory `/path/on/image`.*
Copy link
Member

Choose a reason for hiding this comment

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

Second example is missing the trailing slash!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, will add that.

@coollog coollog requested a review from a team June 29, 2018 19:02
@loosebazooka
Copy link
Member

I just tested copies, and it does become copy (maven knows how to do this)

<copies>
  <copy>asdf</copy>
</copies>

but I'm not sure if this actually reads well.

@coollog
Copy link
Contributor Author

coollog commented Jul 14, 2018

This proposal will probably be retired in favor of the incubating src/main/jib convention.

@TadCordle
Copy link
Contributor

Are there any plans for this proposal, or should we just close this PR?

@coollog
Copy link
Contributor Author

coollog commented Aug 22, 2018

I think this will be good to have as a record in the proposals (and as a reference for the design behind the current solution). I'll touch this up with our current solution (src/main/jib) and move the previous solutions to sections about rejected proposals.

@coollog
Copy link
Contributor Author

coollog commented Aug 22, 2018

Changed the main proposal to regard src/main/jib, moving the previous one to rejected. Also moved this to archives since src/main/jib is already implemented.


The directory should also be able to be configured to override the `src/main/jib` default.

### Alternative (Rejected) Proposal
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to have a ## Alternative Proposals here as they otherwise fall under the ## Proposal.

@coollog coollog merged commit 940684b into master Aug 23, 2018
@coollog coollog deleted the more-layers-proposal branch August 23, 2018 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants