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.

Sign up for GitHub

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

Jump to bottom

Update README for 0.10.0 #1147

Merged
merged 21 commits into from
Nov 9, 2018
Merged

Update README for 0.10.0 #1147

merged 21 commits into from
Nov 9, 2018

Conversation

chanseokoh
Copy link
Member

Fixes #1125.

Should be merged after the next release, so labeling with "PR: Not Ready".

coollog
coollog reviewed Oct 16, 2018
View reviewed changes
jib-gradle-plugin/README.md Outdated Show resolved Hide resolved
jib-gradle-plugin/README.md Outdated Show resolved Hide resolved
@coollog coollog added this to the v0.10.0 milestone Oct 16, 2018
coollog
coollog previously approved these changes Oct 16, 2018
View reviewed changes
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.

Approved for merge after release v0.10.0

@TadCordle TadCordle changed the title Update README for container.user Update README for 0.10.0 Oct 30, 2018
@TadCordle TadCordle dismissed coollog’s stale review October 30, 2018 15:32

Added extraDirectory stuff, should probably re-review.

@TadCordle TadCordle requested a review from a team October 30, 2018 15:39
coollog
coollog reviewed Oct 30, 2018
View reviewed changes
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.

I just realized maybe we should provide a warning if the user set permissions for a file that wasn't an actual path built in the container.

jib-gradle-plugin/README.md Outdated Show resolved Hide resolved
jib-gradle-plugin/README.md Outdated Show resolved Hide resolved
chanseokoh
chanseokoh commented Oct 30, 2018
View reviewed changes
jib-gradle-plugin/README.md Outdated Show resolved Hide resolved
jib-maven-plugin/README.md Outdated Show resolved Hide resolved
jib-gradle-plugin/README.md Outdated Show resolved Hide resolved
jib-maven-plugin/README.md Outdated Show resolved Hide resolved
coollog
coollog reviewed Oct 30, 2018
View reviewed changes
Copy link
Contributor

@coollog coollog left a comment •
edited by TadCordle
Loading

Choose a reason for hiding this comment

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

Looks good! Shall we also add in the other new things to this PR?

  • Properties for each configuration parameter
  • credHelper can be a full path
  • war support
  • jib-image.digest

jib-gradle-plugin/README.md Show resolved Hide resolved
@chanseokoh
Copy link
Member Author

I will do the WAR part.

@TadCordle
Copy link
Contributor

Oh, we should probably address #1138 as well.

@chanseokoh
Copy link
Member Author

Hmm... I feel like it's best to address #1138 after we have the <entrypoint>INHERIT</entrypoint> (and <args>INHERIT</args>.

@TadCordle
Copy link
Contributor

I updated it to be more clear about the current functionality, we can leave the issue open and update it again for 0.10.1.

coollog
coollog reviewed Nov 5, 2018
View reviewed changes
jib-gradle-plugin/README.md Outdated Show resolved Hide resolved
jib-gradle-plugin/README.md Outdated Show resolved Hide resolved
jib-gradle-plugin/README.md Outdated

Jib supports creating images for projects that build a WAR. If the Gradle project uses the [WAR Plugin](https://docs.gradle.org/current/userguide/war_plugin.html), Jib will by default use the [distroless Jetty]https://github.com/GoogleContainerTools/distroless/tree/master/java/jetty) as a base image to deploy the project WAR. No extra configuration is necessary than using the WAR Plugin to make Jib build WAR images.

Note that Jib will work slightly differently for WAR projects from plain Java projects:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is "plain" the right terminology? I think if we do choose a terminology for non-WAR projects, we should stick with it and be consistent everywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really the right terminology. I've been using non-WAR projects, but I remember @briandealwis said (in a different context) "maybe more accurate to say JAR". It'd be good to pick one terminology and stick to it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Going for "JAR projects" instead of "plain Java projects".

@TadCordle
Copy link
Contributor

We should finish/review this since we're releasing. @GoogleContainerTools/java-tools

Does someone more familiar with jib-image.digest want to add something for it?

@chanseokoh
Copy link
Member Author

I'm not so familiar with the image ID and digest honestly. We don't have to add it in this PR, as long as we don't forget to add it soon.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@googlebot googlebot added cla: no and removed cla: yes labels Nov 8, 2018
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@chanseokoh
Copy link
Member Author

LGTM for the parts not from me. Since it's me who put up the PR, this PR needs an approval from others than me.

TadCordle
TadCordle approved these changes Nov 8, 2018
View reviewed changes
Copy link
Contributor

@TadCordle TadCordle left a comment

Choose a reason for hiding this comment

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

LGTM as well

@TadCordle TadCordle merged commit c8f0570 into master Nov 9, 2018
@TadCordle TadCordle deleted the i1125-container.user-doc branch November 9, 2018 15:11
@chanseokoh chanseokoh mentioned this pull request Nov 20, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coollog coollog coollog approved these changes

@TadCordle TadCordle TadCordle approved these changes

Assignees
No one assigned
Labels
cla: yes in progress
Projects
None yet
Milestone
v0.10.0
Development

Successfully merging this pull request may close these issues.
5 participants
@chanseokoh @TadCordle @googlebot @coollog @briandealwis