-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fill in a small amount of details on the jib-as-a-library proposal #1093
Conversation
proposals/jib_core_library.md
Outdated
- `Builder` | ||
- `TarImage saveTo(Path outputFile)` | ||
- `static Builder named(ImageReference/String)` | ||
|
||
`Containerizer` - configures how and where to containerize to | ||
- `static Containerizer to(RegistryImage)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the reasoning for having the static initializers on Containerizer
is to increase discoverability since JibContainerBuilder#containerize
takes a Containerizer
- so it's like of like Streams.of
and then Stream#collect(Collector)
with Collectors.to...
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The examples put these methods on Jib (Jib.to
), not Containerizer
. Will update the examples instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah oops that was a mistake then. Thanks!
proposals/jib_core_library.md
Outdated
|
||
```java | ||
Jib.from("busybox") | ||
// TODO: this doesn't seem right, especially if multiple files are in the .addLayer call (are they cat'ed, or is /helloworld.sh a directory?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pathInContainer
is actually a directory path to place the files under. Maybe it makes more sense to call it directoryInContainer
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
directoryInContainer is good. Updating the example...
Adds a new layer that will consists of the files referred to by `files`, each of which is copied into the `directoryInContainer`. Regardless of the directory nesting of files in `files`, they will be copied into the same level inside the container. If a directory is in the list of `files`, the directory and its contents will be recursively copied into the container | ||
|
||
```java | ||
container.addLayer( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
containerBuilder.addLayer(
looks less confusing.
|
||
- `JibContainerBuilder addLayer(List<Path> files, AbsoluteUnixPath directoryInContainer)` | ||
|
||
Adds a new layer that will consists of the files referred to by `files`, each of which is copied into the `directoryInContainer`. Regardless of the directory nesting of files in `files`, they will be copied into the same level inside the container. If a directory is in the list of `files`, the directory and its contents will be recursively copied into the container |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: period
.setEntrypoint("/helloworld.sh") | ||
.containerize( | ||
Containerizer.to(RegistryImage.named("coollog/jibtestimage") | ||
.setCredential("coollog", "notmyrealpassword"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: mis-aligned indentation
Example: | ||
|
||
```java | ||
RegistryImage destination = RegistryImage.named("gcr.io/myuser/my-java-container:latest"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using gcr.io
, I would say gcr.io/my-cloud-project/...
.
nit: misaligned indentation
```java | ||
RegistryImage destination = RegistryImage.named("gcr.io/myuser/my-java-container:latest"); | ||
|
||
JibContainerBuilder javaImage = JavaContainerBuilder.builder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JavaContainerBuidler javaImage = ...
, since JibContainerBuilder
doesn't have addDependencies()
and such. And rename javaImage
to javaContainerBuilder
or containerBuilder
, since the builder is not an Image
.
@GoogleContainerTools/java-tools I don't remember, will we have JavaContainerBuilder
and methods like addDependencies
, etc?
.addDependencies(Arrays.asList(Paths.get("/my/filesystem/lib/my-dependency.jar"))) | ||
.addClasses(Arrays.asList(Paths.get("/my/filesystem/target/com/google/FooMain.class"))) | ||
.setMainClass("com.google.FooMain") | ||
.toContainerBuilder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already a container builder, so this doesn't sound right.
.addExposedPort(Port.tcp(8080)); | ||
|
||
// Throws an exception if the image couldn't be build. | ||
JibContainer result = javaImage.containerize(Containerizer.to(destination)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
javaImage
-> containerBuilder
, as I said above.
Closing as stale. Thanks for your efforts looking at this. |
Relates to: #337