-
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
Proposal RFC: Jib Core as a library for Java #987
Conversation
- `JibContainerBuilder setExposedPorts(List<Port>/Port...)` | ||
- `JibContainerBuilder addExposedPort(Port port)` | ||
- `JibContainerBuilder setLabels(Map<String, String> labelMap)` | ||
- `JibContainerBuilder addLabel(String key, String value)` |
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.
Weren't we talking about having a more fluent-looking environment()
, labels()
?
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.
Right, that was in #953 - I'll update these to reflect the proposal there.
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.
Actually, after changing this over, I feel like the way we have it now is clearer, since there's a combination of set...
and add...
methods and removing the prefixes may be confusing since most of the set...
methods mean "replace with" rather than "append".
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.
Scratch that - I'll rework this with the setters changed into the ...Only
names.
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.
Hmm, but for setters like setLayers
, layersOnly
or the like doesn't sound right.
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.
And just layers
is unclear as to whether it appends or replaces.
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.
But I don't think we ever replace layers, do we. At least, not for the API. Right?
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.
setLayers
currently would actually replace layers (https://github.com/GoogleContainerTools/jib/blob/master/jib-core/src/main/java/com/google/cloud/tools/jib/api/JibContainerBuilder.java#L109)
Thought about the |
proposals/jib_core_library.md
Outdated
|
||
`JibContainerBuilder` - configures the container to build | ||
- `JibContainerBuilder layer(List<Path> files, Path pathInContainer)` | ||
- `JibContainerBuilder layer(LayerConfiguration)` |
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.
These two were changed to addLayer
, right?
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.
Yep, I'll change this over.
Merged, but final API may be subject to change. |
For #337
Preview at: https://github.com/GoogleContainerTools/jib/blob/jib-core-proposal/proposals/jib_core_library.md
Note that the proposed API is currently in implementation.